Show real app version in container and add cloud favicon #58

Manually merged
marvin8 merged 13 commits from feat/container-version-and-favicon into main 2026-09-30 09:45:28 +00:00
Collaborator

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 ran uv sync --no-install-project, so no yunjin package metadata existed anywhere in the image and importlib.metadata fell back. The builder stage now installs the project, so the version from pyproject.toml lands in the venv and the footer matches a source-checkout run. The code still runs from /app/src via PYTHONPATH (which takes precedence over site-packages), so the alembic.ini-relative init_db behaviour is unchanged. Verified with a scratch venv built both ways: 0.0.0+unknown before, 2026.9.26.1 after, module still resolving from src/.

Cloud favicon (Closes #57)

Adds an SVG favicon rendering the same cloud emoji as the page header, linked from base.html, plus a /favicon.ico redirect so browsers no longer log a 404 on every page. Covered by three new tests in tests/test_web.py (written failing-first).

CI smoke test

The container job now boots the built image and asserts the footer shows the pyproject.toml version, the page head links the favicon, and /favicon.ico resolves — the only automated check at the layer where the version regressed.

Closes #56
Closes #57

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 ran `uv sync --no-install-project`, so no `yunjin` package metadata existed anywhere in the image and `importlib.metadata` fell back. The builder stage now installs the project, so the version from `pyproject.toml` lands in the venv and the footer matches a source-checkout run. The code still runs from `/app/src` via `PYTHONPATH` (which takes precedence over site-packages), so the alembic.ini-relative `init_db` behaviour is unchanged. Verified with a scratch venv built both ways: `0.0.0+unknown` before, `2026.9.26.1` after, module still resolving from `src/`. **Cloud favicon** (Closes #57) Adds an SVG favicon rendering the same cloud emoji as the page header, linked from `base.html`, plus a `/favicon.ico` redirect so browsers no longer log a 404 on every page. Covered by three new tests in `tests/test_web.py` (written failing-first). **CI smoke test** The `container` job now boots the built image and asserts the footer shows the `pyproject.toml` version, the page head links the favicon, and `/favicon.ico` resolves — the only automated check at the layer where the version regressed. Closes #56 Closes #57
The builder stage installed dependencies with --no-install-project, so no
yunjin dist-info existed in the image and importlib.metadata fell back to
the 0.0.0+unknown placeholder in the footer.

Install the project during the build so the version from pyproject.toml
lands in the venv; the code still runs from /app/src via PYTHONPATH, which
takes precedence over site-packages.

Closes #56
Link an SVG favicon that renders the same cloud emoji as the page header,
and redirect the unprompted /favicon.ico request to it so page views stop
logging 404s.

Closes #57
💚 Smoke-test container version and favicon in CI
Some checks failed
/ gitleaks (pull_request) Successful in 15s
/ checks (pull_request) Successful in 1m52s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 3m35s
/ container (pull_request) Failing after 1m43s
178deca8e8
Boot the image built by the container job and assert the footer shows the
pyproject.toml version, the page head links the favicon, and
/favicon.ico resolves — the only check that runs at the layer where the
version placeholder regressed.
forgejo-actions left a comment

WuMing

Found 10 issue(s). See inline comments below.

## WuMing Found **10** issue(s). See inline comments below.
@ -86,1 +86,4 @@
.
- name: Smoke-test built image
run: |
EXPECTED_VERSION=$(grep -oP '^version = "\K[^"]+' pyproject.toml)

config [MEDIUM]

The expected version is extracted with GNU-only grep -P/\K and 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 by set -e) produce an empty value that makes grep "Yunjin v${EXPECTED_VERSION}" match any placeholder. Use a portable extractor such as sed -n 's/^version = "\([^"]*\)"/\1/p' pyproject.toml and assert test -n "$EXPECTED_VERSION" before using it.

**config** [MEDIUM] The expected version is extracted with GNU-only `grep -P`/`\K` and 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 by `set -e`) produce an empty value that makes `grep "Yunjin v${EXPECTED_VERSION}"` match any placeholder. Use a portable extractor such as `sed -n 's/^version = "\([^"]*\)"/\1/p' pyproject.toml` and assert `test -n "$EXPECTED_VERSION"` before using it. <!-- wuming:sha256:5650c80f4eee7150f8e0da82d4d91f85ecb3e84fba13040109af9d5df36750e5 -->
@ -87,0 +87,4 @@
- name: Smoke-test built image
run: |
EXPECTED_VERSION=$(grep -oP '^version = "\K[^"]+' pyproject.toml)
podman run -d --name yunjin-smoke -p 18080:8080 \

config [MEDIUM]

Publishing with -p 18080:8080 binds 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 calls http://localhost:18080/, bind to -p 127.0.0.1:18080:8080 instead.

**config** [MEDIUM] Publishing with `-p 18080:8080` binds 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 calls `http://localhost:18080/`, bind to `-p 127.0.0.1:18080:8080` instead. <!-- wuming:sha256:be40ef8da050e6dbd6227f50b7e309cc4d359a22cd26ffab6c97551629d93ac6 -->

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 rm cleanup, leaving the container running and exposed. Bind to loopback (-p 127.0.0.1:18080:8080) and use --rm or a trap to remove the container on exit. (A05)

**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 rm` cleanup, leaving the container running and exposed. Bind to loopback (`-p 127.0.0.1:18080:8080`) and use `--rm` or a trap to remove the container on exit. (A05) <!-- wuming:sha256:be40ef8da050e6dbd6227f50b7e309cc4d359a22cd26ffab6c97551629d93ac6 -->
@ -87,0 +102,4 @@
podman logs yunjin-smoke || true
exit 1
fi
grep -q "Yunjin v${EXPECTED_VERSION}" /tmp/smoke-page.html || {

config [LOW]

The footer assertion uses grep -q without -F, so the dots in the version are treated as regex wildcards and the match is only a substring. A footer such as Yunjin v2026.9.26.10 would pass for expected version 2026.9.26.1. Use grep -Fq and add a trailing boundary/known suffix to assert the exact version string.

**config** [LOW] The footer assertion uses `grep -q` without `-F`, so the dots in the version are treated as regex wildcards and the match is only a substring. A footer such as `Yunjin v2026.9.26.10` would pass for expected version `2026.9.26.1`. Use `grep -Fq` and add a trailing boundary/known suffix to assert the exact version string. <!-- wuming:sha256:d99caa2b37dbd3c4b216f13307d44f539151f81bd0a4677dea19a5df5982e066 -->
@ -87,0 +114,4 @@
echo "/favicon.ico did not resolve to the SVG icon" >&2
exit 1
}
podman rm -f yunjin-smoke

config [HIGH]

podman rm -f yunjin-smoke runs only after every assertion succeeds. Any failing grep/curl path calls exit 1 and skips cleanup, leaving the container running and port 18080 bound; the next CI run on the same runner will then fail at podman run --name yunjin-smoke with a name/port conflict. Add a trap 'podman rm -f yunjin-smoke' EXIT after starting the container, and remove any stale container before podman run.

**config** [HIGH] `podman rm -f yunjin-smoke` runs only after every assertion succeeds. Any failing `grep`/`curl` path calls `exit 1` and skips cleanup, leaving the container running and port 18080 bound; the next CI run on the same runner will then fail at `podman run --name yunjin-smoke` with a name/port conflict. Add a `trap 'podman rm -f yunjin-smoke' EXIT` after starting the container, and remove any stale container before `podman run`. <!-- wuming:sha256:5fe767186277c4ebd1078772f2dd692202d6a6aeb21410b400783daf590cc5f6 -->
Containerfile Outdated
@ -14,3 +16,2 @@
WORKDIR /build
COPY pyproject.toml uv.lock ./
RUN uv sync --frozen --no-dev --no-install-project
COPY 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 make uv sync fail 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.

**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 make `uv sync` fail 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. <!-- wuming:sha256:b84c08f05b4e1a6c1647bf043de63a961172d8053f9b9c4692289e53cc8fdb1e -->
Containerfile Outdated
@ -15,2 +17,2 @@
COPY pyproject.toml uv.lock ./
RUN uv sync --frozen --no-dev --no-install-project
COPY pyproject.toml uv.lock README.md ./
COPY src/ ./src/

shell [MEDIUM]

The source tree is copied before uv sync, so any change to src/ 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, then COPY src/ ./src/ + RUN uv sync --frozen --no-dev --no-editable (or build a wheel and uv pip install --no-deps it). That keeps the version metadata while making source-only changes cheap to rebuild.

**shell** [MEDIUM] The source tree is copied before `uv sync`, so any change to `src/` 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`, then `COPY src/ ./src/` + `RUN uv sync --frozen --no-dev --no-editable` (or build a wheel and `uv pip install --no-deps` it). That keeps the version metadata while making source-only changes cheap to rebuild. <!-- wuming:sha256:92b538bcafcbcc946953d81f4fcb65bebcb1337a2c602f9d21cacf1f6d44d662 -->
Containerfile Outdated
@ -16,1 +17,3 @@
RUN uv sync --frozen --no-dev --no-install-project
COPY pyproject.toml uv.lock README.md ./
COPY src/ ./src/
RUN uv sync --frozen --no-dev

shell [MEDIUM]

uv sync installs 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-editable so 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.

**shell** [MEDIUM] `uv sync` installs 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-editable` so 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. <!-- wuming:sha256:feb2a1a9e5590b6e10e688ba65859554acb318d0d4f94138fdc0f1b37ee5829b -->
@ -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 add src/yunjin/static/favicon.svg. As shown, /favicon.ico will redirect to a 404 and the new tests/CI smoke test will fail. Add the SVG asset to the PR.

**code** [HIGH] The new route redirects to `/static/favicon.svg`, but the diff does not add `src/yunjin/static/favicon.svg`. As shown, `/favicon.ico` will redirect to a 404 and the new tests/CI smoke test will fail. Add the SVG asset to the PR. <!-- wuming:sha256:0baf674f85a20f20a796f4aa9bf9dbd8dc458adec9708d43c54e75faf431098c -->
marvin8 marked this conversation as resolved
@ -1784,0 +1793,4 @@
assert response.status_code == 200
html = response.data.decode()
assert 'rel="icon"' in html
assert "/static/favicon.svg" in html

code [HIGH]

This test asserts the rendered page contains /static/favicon.svg, but the diff does not update base.html to include the <link rel="icon"> tag. Unless base.html is changed elsewhere, the test will fail.

**code** [HIGH] This test asserts the rendered page contains `/static/favicon.svg`, but the diff does not update `base.html` to include the `<link rel="icon">` tag. Unless `base.html` is changed elsewhere, the test will fail. <!-- wuming:sha256:871cb614435eaa659e8deb3cd9dc13bfaac0d6b5ef64f9baf70acb4be732d8f3 -->
Two-phase uv sync: dependencies install before src/ is copied, so
source-only changes no longer invalidate the dependency layer, and the
project installs with --no-editable so the venv carries a real copy and
its version metadata instead of a dangling /build editable shim.
💚 Smoke-test image contents via buildah instead of podman run
All checks were successful
/ gitleaks (pull_request) Successful in 15s
/ checks (pull_request) Successful in 1m54s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 3m11s
/ container (pull_request) Successful in 1m32s
5b0a621e2d
zhuiri's rootless podman has no subuid/subgid range, so podman run fails
creating container storage for the image's non-root UIDs. Verify through
buildah storage ops — the machinery the build already used: mount the
image and assert the venv dist-info version matches pyproject.toml and
the cloud favicon is shipped. Also switch the version extractor to sed
with an explicit non-empty check.
Author
Collaborator

WuMing wave 1 — responses

Context: the first container run failed because podman run cannot 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 venv dist-info version equals the pyproject.toml version, 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 is sed with an explicit non-empty check (portable, no grep -P); podman run, the published port and the footer grep are gone entirely — the version assertion is now exact string equality between the image's dist-info METADATA and pyproject.toml, and the working container is removed via an EXIT trap plus explicit umount/rm. (#3939 carries the same wuming:sha256 as #3933, so it is the same finding; answered once here.)

✅ Fixed — Containerfile build

#3936 · #3937

Fixed in commit 6cd1ec1: two-phase uv sync — dependencies install before src/ is copied (layer caching restored), then the project installs with --no-editable, writing a real copy plus its dist-info into the venv instead of a dangling /build editable shim.

🔴 Not actioned — favicon.svg missing from the diff

#3930

False positive: src/yunjin/web/static/favicon.svg is added in commit 5461c0f (note the web/ path segment). The checks job on this PR runs the three new favicon tests on Python 3.12–3.14 — all green.

#3931

False positive: the <link rel="icon" ...> tag was added to base.html in the same commit 5461c0f; the rendered-page test asserting it passes in the green checks job.

🔴 Not actioned — copy the complete set of metadata files

#3938

pyproject.toml references 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 sync fails loudly at build time (as the comment predicts), which the container job would catch immediately; speculative pre-copying would just rot.

### WuMing wave 1 — responses Context: the first `container` run failed because **`podman run` cannot 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 venv `dist-info` version equals the `pyproject.toml` version, 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3932) · [#3933](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3933) · [#3934](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3934) · [#3935](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3935) · [#3939](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3939) Fixed/superseded in commit `5b0a621`: the version extractor is `sed` with an explicit non-empty check (portable, no `grep -P`); `podman run`, the published port and the footer grep are gone entirely — the version assertion is now exact string equality between the image's `dist-info` METADATA and `pyproject.toml`, and the working container is removed via an `EXIT` trap plus explicit `umount`/`rm`. ([#3939](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3939) carries the same `wuming:sha256` as [#3933](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3933), so it is the same finding; answered once here.) ### ✅ Fixed — Containerfile build [#3936](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3936) · [#3937](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3937) Fixed in commit `6cd1ec1`: two-phase `uv sync` — dependencies install before `src/` is copied (layer caching restored), then the project installs with `--no-editable`, writing a real copy plus its `dist-info` into the venv instead of a dangling `/build` editable shim. ### 🔴 Not actioned — favicon.svg missing from the diff [#3930](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3930) False positive: `src/yunjin/web/static/favicon.svg` **is** added in commit `5461c0f` (note the `web/` path segment). The `checks` job 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3931) False positive: the `<link rel="icon" ...>` tag was added to `base.html` in the same commit `5461c0f`; the rendered-page test asserting it passes in the green `checks` job. ### 🔴 Not actioned — copy the complete set of metadata files [#3938](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3938) `pyproject.toml` references 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 sync` fails loudly at build time (as the comment predicts), which the container job would catch immediately; speculative pre-copying would just rot.
forgejo-actions left a comment

WuMing

Found 5 issue(s). See inline comments below.

## WuMing Found **5** issue(s). See inline comments below.
Containerfile Outdated
@ -13,3 +11,3 @@
WORKDIR /build
COPY pyproject.toml uv.lock ./
COPY pyproject.toml uv.lock README.md ./

shell [LOW]

README.md is copied into the dependency-install layer (line 22), but it is only needed by the project build at line 24 (because pyproject.toml declares 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 before RUN uv sync --frozen --no-dev --no-editable, so only the project-install layer is invalidated.

**shell** [LOW] `README.md` is copied into the dependency-install layer (line 22), but it is only needed by the project build at line 24 (because `pyproject.toml` declares 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 before `RUN uv sync --frozen --no-dev --no-editable`, so only the project-install layer is invalidated. <!-- wuming:sha256:84181bf039c0729c62a29ed4b373bffcca2d772a1b9d89073c1de857bdcd9c31 -->
@ -16,1 +21,4 @@
# relative to the source tree root.
RUN uv sync --frozen --no-dev --no-install-project
COPY src/ ./src/
RUN uv sync --frozen --no-dev --no-editable

shell [MEDIUM]

Installing the project non-editable copies the whole yunjin package 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 on PYTHONPATH winning 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. Set PYTHONPATH=/app/src via ENV in the runtime stage rather than only in the entrypoint/CMD so it is inherited unconditionally, and consider adding a startup assertion (or test) that yunjin.__file__ resolves under /app/src.

**shell** [MEDIUM] Installing the project non-editable copies the whole `yunjin` package 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 on `PYTHONPATH` winning 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. Set `PYTHONPATH=/app/src` via `ENV` in the runtime stage rather than only in the entrypoint/CMD so it is inherited unconditionally, and consider adding a startup assertion (or test) that `yunjin.__file__` resolves under `/app/src`. <!-- wuming:sha256:e3732826d0ebfb71135fe61292c3b8ee8773033f8fcdc4a4991b249036e5a124 -->
marvin8 marked this conversation as resolved
@ -14,1 +14,4 @@
The build installs the project's package metadata (version from
`pyproject.toml`) alongside the locked dependencies, so the web footer
reports the real application version — e.g. `Yunjin v2026.9.26.1` — the

docs [LOW]

The example version Yunjin v2026.9.26.1 is a concrete, date-based version pinned into prose. It will become stale (and potentially misleading) as soon as the next release bumps pyproject.toml, and it also reads as a future-dated version for the current release schedule. Prefer a neutral placeholder such as Yunjin v<version> or a note that the value tracks pyproject.toml, so the doc doesn't require editing on every version bump.

**docs** [LOW] The example version `Yunjin v2026.9.26.1` is a concrete, date-based version pinned into prose. It will become stale (and potentially misleading) as soon as the next release bumps `pyproject.toml`, and it also reads as a future-dated version for the current release schedule. Prefer a neutral placeholder such as `Yunjin v<version>` or a note that the value tracks `pyproject.toml`, so the doc doesn't require editing on every version bump. <!-- wuming:sha256:ea60d76a1ab41dafa3ddca45a57a2df7f3f03b4b922cedd6334a5790c9d980ba -->
@ -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.

**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. <!-- wuming:sha256:6708e8ed3591f4f71f88dd60917c5793ebbbdadfc3bb040b5dd0af8c8f1b61d1 -->
marvin8 marked this conversation as resolved
@ -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.

**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:sha256:01fab01496b2f94e14f82877d6871b81ceb431885e698a4bab7cfb31bbbc4c23 -->
🔧 Address WuMing review wave on PR #58
Some checks failed
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 2m3s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Failing after 1m3s
/ pr-review (pull_request) Successful in 3m40s
43e86975c0
- Move README.md into the project-install layer so doc edits don't
  invalidate the dependency-install layer.
- Assert in the container smoke test that yunjin resolves from /app/src
  under PYTHONPATH, never from the metadata copy in the venv.
- Narrow the favicon test docstring to what it actually exercises.
- Use a neutral version placeholder in the container docs.
Author
Collaborator

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.svg is added in commit 5461c0f, and the favicon tests pass in the green checks job 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.md now uses a neutral Yunjin v<version> placeholder.

✅ Actioned — import resolution must come from /app/src

#3946

One premise is already true: PYTHONPATH=/app/src is set via ENV in 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 runs python -c "import yunjin; assert yunjin.__file__ == '/app/src/yunjin/__init__.py'" inside the image (buildah, chroot isolation) so a divergence would fail CI loudly. Commit 43e8697.

✅ Fixed — README.md invalidating the dependency layer

#3947

Fixed in commit 43e8697: README.md moved out of the dependency-install COPY; it now lands with src/ in the project-install layer, so README edits only invalidate the cheap layer.

### WuMing wave 2 — responses ### 🔴 Duplicate of wave-1 finding — favicon.svg "not in the diff" [#3943](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3943) Identical to [#3930](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3930), answered above: `src/yunjin/web/static/favicon.svg` is added in commit `5461c0f`, and the favicon tests pass in the green `checks` job on this PR. The comment reappeared only because the diff shifted; nothing to do. ### ✅ Fixed — docstring overclaims page coverage [#3944](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3945) Fixed in commit `43e8697`: `docs/container.md` now uses a neutral `Yunjin v<version>` placeholder. ### ✅ Actioned — import resolution must come from /app/src [#3946](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3946) One premise is already true: `PYTHONPATH=/app/src` is set via `ENV` in 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 runs `python -c "import yunjin; assert yunjin.__file__ == '/app/src/yunjin/__init__.py'"` inside the image (buildah, chroot isolation) so a divergence would fail CI loudly. Commit `43e8697`. ### ✅ Fixed — README.md invalidating the dependency layer [#3947](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3947) Fixed in commit `43e8697`: `README.md` moved out of the dependency-install `COPY`; it now lands with `src/` in the project-install layer, so README edits only invalidate the cheap layer.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -86,1 +86,4 @@
.
- name: Smoke-test built image
run: |
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 with version = ". Any other table that carries a version key (e.g. [tool.commitizen], VCS/release tooling) or a future reordering of the file would silently produce a different EXPECTED_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).

**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 with `version = "`. Any other table that carries a `version` key (e.g. `[tool.commitizen]`, VCS/release tooling) or a future reordering of the file would silently produce a different `EXPECTED_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). <!-- wuming:sha256:c9f1c7c627221362132c40eb2e9c09b324571a1782a4470e87716bc3667e71eb -->
@ -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 — that base.html actually links /favicon.svg and that /favicon.ico resolves 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. a TestClient call in the smoke test, or a grep of the router registration).

**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 — that `base.html` actually links `/favicon.svg` and that `/favicon.ico` resolves 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. a `TestClient` call in the smoke test, or a grep of the router registration). <!-- wuming:sha256:9123d83f8c3338308556a7bb3d8653916122224c8f048b295c93fa77c329a9a0 -->
@ -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 run cannot 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 via buildah 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. Confirm buildah run --isolation chroot actually executes on zhuiri (e.g. wrap it in buildah unshare) and update the comment, otherwise the smoke step will break the pipeline.

**config** [LOW] Lines 94–98 state that `podman run` cannot 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 via `buildah 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. Confirm `buildah run --isolation chroot` actually executes on zhuiri (e.g. wrap it in `buildah unshare`) and update the comment, otherwise the smoke step will break the pipeline. <!-- wuming:sha256:4d5c8d6fa6e40ae22c4fedd69b9acb4bb56179585cbb201886ea83131051dfa7 -->
marvin8 marked this conversation as resolved
🐛 Keep src/ nesting in the project-install layer and harden smoke test
All checks were successful
/ gitleaks (pull_request) Successful in 16s
/ checks (pull_request) Successful in 1m50s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Successful in 1m25s
/ pr-review (pull_request) Successful in 3m54s
95135b2932
COPY src/ README.md ./ flattened src/yunjin into /build/yunjin, so
uv_build found no module at src/yunjin/__init__.py and the build failed.
Copy the tree and the readme separately.

Also scope the expected-version extraction to the [project] table and
assert the favicon link and redirect route in the baked image tree.
Author
Collaborator

WuMing wave 3 — responses

Context: run 121 failed before buildah run ever executed — my previous commit broke the build itself: COPY src/ README.md ./ copies the contents of src/, flattening src/yunjin to /build/yunjin, so uv_build found no module at src/yunjin/__init__.py. Fixed in commit 95135b2 by copying the tree and the readme as separate COPY instructions. 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 run does not: podman run needs a user namespace to map the image's non-root UIDs (none available rootless on zhuiri), while buildah with --isolation chroot never creates a user namespace. The next CI run is the first actual execution of buildah run on 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 awk snippet that only reads a version = key inside the [project] table (verified against pyproject.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.html for the favicon.svg link and the baked web/__init__.py for the favicon.ico redirect route. End-to-end request behaviour (template render, 302 chain) stays covered by the pytest suite in the checks job; the container job now verifies all of it shipped inside the image.

### WuMing wave 3 — responses Context: run 121 failed **before** `buildah run` ever executed — my previous commit broke the build itself: `COPY src/ README.md ./` copies the *contents* of `src/`, flattening `src/yunjin` to `/build/yunjin`, so `uv_build` found no module at `src/yunjin/__init__.py`. Fixed in commit `95135b2` by copying the tree and the readme as separate `COPY` instructions. All three findings below are also actioned in the same commit. ### ✅ Fixed — buildah run rationale / will it execute? [#3954](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3954) The comment now states precisely why chroot isolation works where `podman run` does not: `podman run` needs a user namespace to map the image's non-root UIDs (none available rootless on zhuiri), while `buildah` with `--isolation chroot` never creates a user namespace. The next CI run is the first actual execution of `buildah run` on 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3952) The extraction is now an `awk` snippet that only reads a `version = ` key inside the `[project]` table (verified against `pyproject.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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3953) The smoke step now also greps the baked `base.html` for the `favicon.svg` link and the baked `web/__init__.py` for the `favicon.ico` redirect route. End-to-end request behaviour (template render, 302 chain) stays covered by the pytest suite in the `checks` job; the container job now verifies all of it shipped inside the image.
forgejo-actions left a comment

WuMing

Found 7 issue(s). See inline comments below.

## WuMing Found **7** issue(s). See inline comments below.
@ -86,1 +86,4 @@
.
- name: Smoke-test built image
run: |
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 $3 being 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.

**config** [LOW] This hand-rolled awk parser duplicates TOML parsing in CI and depends on `$3` being 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. <!-- wuming:sha256:b6287765b0f9a6188606e5df60b8db9160fb91b0850341ad991e7086801ae858 -->
marvin8 marked this conversation as resolved
@ -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 || :' EXIT

config [LOW]

The EXIT trap only removes the container; it does not unmount the rootfs created by buildah mount on 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 (and buildah rm -f on a mounted container can itself fail, so the || : hides cleanup failure). Make the trap also buildah umount before buildah rm, e.g. trap 'buildah umount "$ctr" >/dev/null 2>&1 || :; buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXIT.

**config** [LOW] The EXIT trap only removes the container; it does not unmount the rootfs created by `buildah mount` on 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 (and `buildah rm -f` on a mounted container can itself fail, so the `|| :` hides cleanup failure). Make the trap also `buildah umount` before `buildah rm`, e.g. `trap 'buildah umount "$ctr" >/dev/null 2>&1 || :; buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXIT`. <!-- wuming:sha256:3c8cb91f8b42995dc9542885ecf53440a41801b14af992a3fa58e231f9601a0d -->
@ -87,0 +100,4 @@
trap 'buildah rm -f "$ctr" >/dev/null 2>&1 || :' EXIT
rootfs=$(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-info left next to the real one) and head -1 then silently trusts whichever sed emits first; conversely, if the glob matches nothing, sed fails on a literal path and the only diagnosis is a confusing image 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.

**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-info` left next to the real one) and `head -1` then silently trusts whichever `sed` emits first; conversely, if the glob matches nothing, `sed` fails on a literal path and the only diagnosis is a confusing `image 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. <!-- wuming:sha256:24fa98b065d35adabaa96b9f713cc171e2b6a77167f635f2f47ef4093c819bcc -->
@ -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" ]; then

config [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-rc1 or 1.0.0+local is stored/rendered as 1.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 with buildah 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).

**config** [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-rc1` or `1.0.0+local` is stored/rendered as `1.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 with `buildah 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). <!-- wuming:sha256:091259918245da46a226243492a2ffe85f970bcda5279d29a246ae56a35efc63 -->
marvin8 marked this conversation as resolved
@ -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.ico actually 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 (or buildah run the app with the test DB) and curl -fsS localhost:PORT/ expecting the version string in the body, plus curl -sI localhost:PORT/favicon.ico expecting a 3xx to favicon.svg.

**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.ico` actually 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 (or `buildah run` the app with the test DB) and `curl -fsS localhost:PORT/` expecting the version string in the body, plus `curl -sI localhost:PORT/favicon.ico` expecting a 3xx to favicon.svg. <!-- wuming:sha256:54b19e55c189765563c8aaae76a2ad280c965de0e6b5080616b4f065a071dace -->
marvin8 marked this conversation as resolved
@ -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.

**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. <!-- wuming:sha256:b16c675febc6652000b8e8a04b666eab3ea44e110be45f91088c1cac7bccd5ff -->
@ -16,1 +22,4 @@
RUN uv sync --frozen --no-dev --no-install-project
COPY src/ ./src/
COPY README.md ./
RUN uv sync --frozen --no-dev --no-editable

shell [MEDIUM]

Installing the project with --no-editable writes 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 (a docker 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: set ENV PYTHONPATH=/app/src in 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.

**shell** [MEDIUM] Installing the project with `--no-editable` writes 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 (a `docker 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: set `ENV PYTHONPATH=/app/src` in 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:sha256:0392b3262cec41ba2506964f80d9092e46f973d848e075ac43d4864a4ded1d55 -->
marvin8 marked this conversation as resolved
💚 Boot the app in the container smoke test
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 1m51s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Successful in 1m38s
/ pr-review (pull_request) Successful in 3m49s
2cf562b6a7
Verify the user-visible behaviours for real: boot the web entrypoint on
an uncommon port inside the image and assert the rendered footer shows
the pyproject version and /favicon.ico 302s to the SVG. The import
assertion no longer overrides PYTHONPATH, so a forgotten image ENV would
fail it, and the config env is inspected explicitly. Cleanup trap now
kills the server and umounts before removing the working container.
Static template/route greps are superseded by the live checks.
Author
Collaborator

WuMing wave 4 — responses

CI run 122 for the previous push was fully green, including the first real buildah run execution on zhuiri — the chroot rationale holds. This wave splits into four fixes (commit 2cf562b) 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 altered ENV PYTHONPATH fails it. Belt-and-braces: the step also buildah inspects the image config env for PYTHONPATH=/app/src before anything else.

✅ Fixed — boot the app and verify the real behaviours

#3962

The smoke step now boots the actual web entrypoint inside the image (buildah run, chroot isolation, PORT=18099) and asserts: rendered footer contains Yunjin v<pyproject version> | (literal grep -F), /favicon.ico answers 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 cleanup trap now kills the booted server (if any), buildah umounts, and only then buildah 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 the buildah inspect config-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. Comparing importlib.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 sync installs exactly one yunjin distribution (the build log shows the single + yunjin==… line). A missing match already fails the step loudly — sed on the unmatched literal path exits non-zero under the runner's set -e. (One concession folded into 2cf562b anyway: 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. A python3-with-tomllib dependency on the runner is not guaranteed the way POSIX awk is, and the repo's TOML is uniformly key = value formatted, so the fragility is bounded to loud failures. Declined on that trade-off.

### WuMing wave 4 — responses CI run 122 for the previous push was fully green, including the first real `buildah run` execution on zhuiri — the chroot rationale holds. This wave splits into four fixes (commit `2cf562b`) and three declines with evidence. ### ✅ Fixed — PYTHONPATH assertion can't catch a forgotten ENV [#3961](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3961) Sharp catch. The import assertion no longer overrides `PYTHONPATH` — it now runs with the image's own environment, so a forgotten or altered `ENV PYTHONPATH` fails it. Belt-and-braces: the step also `buildah inspect`s the image config env for `PYTHONPATH=/app/src` before anything else. ### ✅ Fixed — boot the app and verify the real behaviours [#3962](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3962) The smoke step now boots the actual `web` entrypoint inside the image (`buildah run`, chroot isolation, `PORT=18099`) and asserts: rendered footer contains `Yunjin v<pyproject version> |` (literal `grep -F`), `/favicon.ico` answers **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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3963) The `cleanup` trap now kills the booted server (if any), `buildah umount`s, and only then `buildah rm -f`s the working container. ### ✅ Fixed — import/ENV invariant made explicit [#3964](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3961)'s fix and is now in, including the `buildah inspect` config-env check you suggested. ### 🔴 Not actioned — PEP 440 normalization of the version comparison [#3958](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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. Comparing `importlib.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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3959) Two yunjin dist-infos cannot exist: the venv is created fresh in the image build and `uv sync` installs exactly one yunjin distribution (the build log shows the single `+ yunjin==…` line). A missing match already fails the step loudly — `sed` on the unmatched literal path exits non-zero under the runner's `set -e`. (One concession folded into `2cf562b` anyway: 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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. A `python3`-with-`tomllib` dependency on the runner is not guaranteed the way POSIX awk is, and the repo's TOML is uniformly `key = value` formatted, so the fragility is bounded to loud failures. Declined on that trade-off.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## 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 inspect defaults 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" (.OCIv1 is the image-inspect field anyway).

**config** [MEDIUM] `buildah inspect` defaults 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"` (`.OCIv1` is the image-inspect field anyway). <!-- wuming:sha256:e879a3112bdbe257ecb38232bd09be50bce46de2bdb48314be7f3561e8868440 -->
marvin8 marked this conversation as resolved
@ -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/FastAPI RedirectResponse defaults to 307 (308/301 are equally valid for a canonical favicon redirect), so if the handler does not explicitly set status_code=302 the job fails even though /favicon.ico resolves 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-up curl -fsSL ... | grep -q '<svg' verify the actual content.

**config** [LOW] The smoke test asserts the redirect is exactly `302`. That couples CI to an implementation detail: Starlette/FastAPI `RedirectResponse` defaults to 307 (308/301 are equally valid for a canonical favicon redirect), so if the handler does not explicitly set `status_code=302` the job fails even though `/favicon.ico` resolves 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-up `curl -fsSL ... | grep -q '<svg'` verify the actual content. <!-- wuming:sha256:61f0b42cdb55b18311dc6ec76c5093c3e30fa13dd0d9642c4528f9f4922036e6 -->
marvin8 marked this conversation as resolved
@ -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-project

shell [LOW]

Both uv sync invocations 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 in uv.lock; the image build is therefore neither hermetic nor offline-reproducible, which is at odds with the UV_PYTHON_DOWNLOADS=never intent. Use RUN --mount=type=cache,target=/root/.cache/uv uv sync ... and pin the build backend (or use --no-build-isolation with the build requirements installed from the lock) so the CI container job is deterministic.

**shell** [LOW] Both `uv sync` invocations 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 in `uv.lock`; the image build is therefore neither hermetic nor offline-reproducible, which is at odds with the `UV_PYTHON_DOWNLOADS=never` intent. Use `RUN --mount=type=cache,target=/root/.cache/uv uv sync ...` and pin the build backend (or use `--no-build-isolation` with the build requirements installed from the lock) so the CI container job is deterministic. <!-- wuming:sha256:2dae876f9edec8aee0d66812ad7cde84dda3b7fa7ee98e46cc0bdba6a5bbf80f -->
marvin8 marked this conversation as resolved
Author
Collaborator

WuMing wave 5 — responses (all declined, no code change)

CI run 744 on 2cf562b is fully green — including the buildah 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 pinning status_code=302 in 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=cache is BuildKit syntax, not what drives caching here: buildah layer caching already prevents re-downloads — phase 2's uv sync reuses 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 the checks job's own uv build, which is the project's existing standard.

### WuMing wave 5 — responses (all declined, no code change) CI run 744 on `2cf562b` is fully green — including the `buildah inspect`, the no-override import assertion, and the live boot checks this wave comments on. ### 🔴 Not actioned — buildah inspect type [#3968](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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 pinning `status_code=302` in the handler or widening the check would add noise without changing what is guaranteed. ### 🔴 Not actioned — BuildKit cache mounts / hermetic build [#3970](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3970) `RUN --mount=type=cache` is BuildKit syntax, not what drives caching here: buildah layer caching already prevents re-downloads — phase 2's `uv sync` reuses 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 the `checks` job's own `uv build`, which is the project's existing standard.
🐛 Centre the favicon cloud so it does not clip
All checks were successful
/ gitleaks (pull_request) Successful in 23s
/ checks (pull_request) Successful in 1m56s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m29s
/ container (pull_request) Successful in 1m36s
cb64dc6046
The emoji glyph at font-size 90 started at x=0 in a 100-unit viewBox and
its advance width overflowed the right edge, clipping the cloud. Centre
the text on a 128-unit canvas with headroom for wide emoji fonts, and
centre it vertically with dominant-baseline.
forgejo-actions left a comment

WuMing

Found 1 issue(s). See inline comments below.

## WuMing Found **1** issue(s). See inline comments below.
@ -1784,0 +1808,4 @@
assert response.status_code == 302
assert 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.

**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. <!-- wuming:sha256:b5061a0c78cd22ea7f1294ddf38efbc8cf9345705c1b5409a67ad72735fabf5f -->
marvin8 approved these changes 2026-09-30 08:48:55 +00:00
Dismissed
✅ Follow the actual redirect Location in the favicon test
All checks were successful
/ gitleaks (pull_request) Successful in 13s
/ checks (pull_request) Successful in 1m55s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Successful in 1m42s
/ pr-review (pull_request) Successful in 3m53s
d316b05d9a
A fresh follow_redirects request exercises the route twice but never
proves the original 302's Location is reachable; request the returned
Location directly instead.
agent-pi dismissed marvin8's review 2026-09-30 08:52:00 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Author
Collaborator

WuMing wave 6 — responses

✅ Fixed — follow the returned Location, not a fresh request

#3974

Fixed in commit d316b05: the test now requests response.headers["Location"] directly, so it verifies that the exact Location returned by the 302 is reachable.

### WuMing wave 6 — responses ### ✅ Fixed — follow the returned Location, not a fresh request [#3974](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3974) Fixed in commit `d316b05`: the test now requests `response.headers["Location"]` directly, so it verifies that the exact Location returned by the 302 is reachable.
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## 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]}.

**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]}. <!-- wuming:sha256:012e3c81e37a085da835792153f7e4e0e9cce4a8530d48f66543320132bbd0fd -->
marvin8 marked this conversation as resolved
@ -87,0 +153,4 @@
cat /tmp/yunjin-smoke.log >&2 || :
exit 1
fi
printf '%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; }.

**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 <link rel="icon"> 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:sha256:3659f7780f6ccbbd436063fc0162c7732d5dbb395c2fc7b27d300b38e5e8c4d5 -->
marvin8 marked this conversation as resolved
Author
Collaborator

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 for favicon.svg alongside 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 sync installs exactly one yunjin distribution, so a second yunjin-*.dist-info cannot exist; a missing match already fails loudly. The re-raised marker differs only because the diff moved.

### WuMing wave 7 — responses ### ✅ Fixed — page-head favicon claim not actually checked [#3980](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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 for `favicon.svg` alongside the footer assertion. ### 🔴 Duplicate of wave-4 finding — dist-info glob multi-match [#3981](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3981) Same substance as [#3959](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3959), answered there: the venv is created fresh in this build and `uv sync` installs exactly one yunjin distribution, so a second `yunjin-*.dist-info` cannot exist; a missing match already fails loudly. The re-raised marker differs only because the diff moved.
💚 Assert the fetched page links the favicon in the smoke test
All checks were successful
/ gitleaks (pull_request) Successful in 24s
/ checks (pull_request) Successful in 1m49s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m29s
/ container (pull_request) Successful in 1m39s
2354a916c7
The PR claims the smoke test checks the page head links the favicon;
restore that coverage with a live check on the rendered page.
forgejo-actions left a comment

WuMing

Found 7 issue(s). See inline comments below.

## WuMing Found **7** issue(s). See inline comments below.
@ -86,1 +86,4 @@
.
- name: Smoke-test built image
run: |
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"])').

**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"])')`. <!-- wuming:sha256:5dfd340eeb35ec0b5fe15f3d6ce80a00b3693e19333b6a1554e5d90fe6ead34e -->
marvin8 marked this conversation as resolved
@ -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 inspect defaults to object type container, 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. Use buildah inspect --type image --format '{{.OCIv1.Config.Env}}' "$IMAGE" (or podman inspect).

**config** [HIGH] `buildah inspect` defaults to object type `container`, 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. Use `buildah inspect --type image --format '{{.OCIv1.Config.Env}}' "$IMAGE"` (or `podman inspect`). <!-- wuming:sha256:cd5ccce7c3cde8e2588582d9782e3331fdbb36d82cf6ae13de579485427164eb -->
marvin8 marked this conversation as resolved
@ -87,0 +107,4 @@
server_pid=
cleanup() {
if [ -n "$server_pid" ]; then
kill "$server_pid" 2>/dev/null || :

config [MEDIUM]

This only kills the buildah run client 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 shared zhuiri runner. 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 with setsid and reap the whole group (kill -- -"$server_pid"), or pkill -f container-entrypoint.sh in cleanup.

**config** [MEDIUM] This only kills the `buildah run` client 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 shared `zhuiri` runner. 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 with `setsid` and reap the whole group (`kill -- -"$server_pid"`), or `pkill -f container-entrypoint.sh` in cleanup. <!-- wuming:sha256:6cd80cde0fb816c36738ac3fbec92cfc1679ea4fe56843671dfc9e11a8e3d077 -->
@ -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 with ss) and pass it through to the entrypoint, the readiness loop, and the favicon checks.

**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 with `ss`) and pass it through to the entrypoint, the readiness loop, and the favicon checks. <!-- wuming:sha256:0da15eda3a0c5df11ffea2a0f67e938adaf891d32947b9854ff8cca7487b6c5e -->
marvin8 marked this conversation as resolved
@ -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 from

docs [LOW]

Completeness: the paragraph explains which metadata is installed but not which code runs. Per the change, the application still imports from /app/src via PYTHONPATH (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/src at runtime; only the distribution metadata is installed") so the container docs match the actual runtime behaviour described in the PR.

**docs** [LOW] Completeness: the paragraph explains which metadata is installed but not which code runs. Per the change, the application still imports from `/app/src` via `PYTHONPATH` (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/src` at runtime; only the distribution metadata is installed") so the container docs match the actual runtime behaviour described in the PR. <!-- wuming:sha256:0b785d00939776e0c38983bbcb036a8444339bbc65a111226763a41ff8fbccbd -->
marvin8 marked this conversation as resolved
@ -1784,0 +1793,4 @@
assert response.status_code == 200
html = response.data.decode()
assert 'rel="icon"' in html
assert "/static/favicon.svg" in html

code [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.

**code** [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. <!-- wuming:sha256:70f3dea487d4bb52a23f3aeb2262811f067aa5634702271efdf019a9a0638ab7 -->
@ -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.

**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:sha256:d41c3d00617d416a4672bf460426ad3bb5ed7072d16ce572028a0f10a94d2fe7 -->
🐛 Kill the smoke-test server's process group, document what runs
All checks were successful
/ gitleaks (pull_request) Successful in 23s
/ checks (pull_request) Successful in 1m52s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ container (pull_request) Successful in 1m39s
/ pr-review (pull_request) Successful in 3m35s
f12f1a9a6b
setsid the booted server so cleanup signals the whole tree instead of
orphaning the exec'd web server, and state in the container docs that the
code still runs from /app/src via PYTHONPATH.
Author
Collaborator

WuMing wave 8 — responses

✅ Fixed — orphaned smoke-test server

#3988

Fixed in commit f12f1a9: the server boots under setsid, 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.md now states the application code is still imported from /app/src at runtime (PYTHONPATH takes 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.html link are "not in the diff": both are added in commit 5461c0f (src/yunjin/web/static/favicon.svg, <link rel="icon">), and the favicon tests have passed in every green checks job since — including run 747 on the current head.

#3987

Duplicate of #3968, answered there: the buildah inspect line 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 wave 8 — responses ### ✅ Fixed — orphaned smoke-test server [#3988](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3988) Fixed in commit `f12f1a9`: the server boots under `setsid`, 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3991) Fixed in commit `f12f1a9`: `docs/container.md` now states the application code is still imported from `/app/src` at runtime (`PYTHONPATH` takes precedence over site-packages) and the installed copy only carries metadata. ### 🔴 Duplicates — re-raised findings already answered [#3985](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3985) · [#3986](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3986) Third re-raise of the wave-1 finding that the SVG asset and the `base.html` link are "not in the diff": both are added in commit `5461c0f` (`src/yunjin/web/static/favicon.svg`, `<link rel="icon">`), and the favicon tests have passed in every green `checks` job since — including run 747 on the current head. [#3987](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3987) Duplicate of [#3968](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3968), answered there: the `buildah inspect` line 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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3990) Duplicate of [#3960](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3988)'s process-group kill now guarantees this job cleans up after itself too.
forgejo-actions left a comment

WuMing

Found 5 issue(s). See inline comments below.

## WuMing Found **5** issue(s). See inline comments below.
@ -86,1 +86,4 @@
.
- name: Smoke-test built image
run: |
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] (setting inproj=1 again and skipping the reset rule), and $3 assumes exactly the form version = "x" with whitespace on both sides of =. A line written as version="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\]$/.

**config** [LOW] The version extraction is fragile. `/^\[project\]/` is not end-anchored, so it also matches `[project.optional-dependencies]` / `[project.scripts]` (setting `inproj=1` again and skipping the reset rule), and `$3` assumes exactly the form `version = "x"` with whitespace on both sides of `=`. A line written as `version="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\]$/`. <!-- wuming:sha256:56cbc4c20eecd68bd3e4adc430af65ee6bbe4a398f1c454c9af4fc843a3cb765 -->
marvin8 marked this conversation as resolved
@ -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 whole Config.Env slice, so it would also pass for PYTHONPATH=/app/src-other or for an unrelated variable such as MY_PYTHONPATH=/app/src. Anchor the match to the exact env entry (e.g. grep for PYTHONPATH=/app/src including a trailing space/bracket, or inspect the env list programmatically) so a wrong PYTHONPATH cannot silently satisfy the check.

**config** [LOW] `grep -q 'PYTHONPATH=/app/src'` is an unanchored substring match against the whole `Config.Env` slice, so it would also pass for `PYTHONPATH=/app/src-other` or for an unrelated variable such as `MY_PYTHONPATH=/app/src`. Anchor the match to the exact env entry (e.g. grep for `PYTHONPATH=/app/src ` including a trailing space/bracket, or inspect the env list programmatically) so a wrong PYTHONPATH cannot silently satisfy the check. <!-- wuming:sha256:e445abede1b811aab11bd928058c1f3879b92df860e86668280d350db70604b9 -->
marvin8 marked this conversation as resolved
@ -87,0 +122,4 @@
exit 1
fi
installed=$(sed -n 's/^Version: \(.*\)$/\1/p' "$metadata")
if [ "$installed" != "$EXPECTED_VERSION" ]; then

config [LOW]

This compares the raw pyproject.toml string against Version: in dist-info METADATA, which is the PEP 440-normalized form. For values like 2026.09.26.1, 1.0-rc1 or 1.0+BUILD the two differ and the smoke test fails even though the built image is correct. Compare against the same source the footer uses (e.g. run python -c "import importlib.metadata;print(importlib.metadata.version('yunjin'))" inside the image and assert it is non-placeholder), or normalize both sides with packaging.version.Version before comparing.

**config** [LOW] This compares the raw pyproject.toml string against `Version:` in dist-info METADATA, which is the PEP 440-normalized form. For values like `2026.09.26.1`, `1.0-rc1` or `1.0+BUILD` the two differ and the smoke test fails even though the built image is correct. Compare against the same source the footer uses (e.g. run `python -c "import importlib.metadata;print(importlib.metadata.version('yunjin'))"` inside the image and assert it is non-placeholder), or normalize both sides with `packaging.version.Version` before comparing. <!-- wuming:sha256:bf0d2160a03a1707b4bbe9cb762a8916ea90a5e9aaa3864a1dab760777b3dc67 -->
marvin8 marked this conversation as resolved
@ -87,0 +146,4 @@
server_pid=$!
page=""
for attempt in $(seq 1 30); do
if page=$(curl -fsS http://localhost:18099/ 2>/dev/null); then

config [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).

**config** [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). <!-- wuming:sha256:4a443cca51ca0740d47dd91a3fbd9477f296ed9da8fc9fd1f19039ff850cde0e -->
@ -15,0 +17,4 @@
reports the real application version (`Yunjin v<version>`) — the same way
a source-checkout run does. The application code itself is still imported
from `/app/src` at runtime (`PYTHONPATH` takes precedence over
site-packages); the installed copy only carries the metadata.

docs [LOW]

This sentence is slightly inaccurate: uv sync without --no-install-project writes a full copy of the yunjin modules into site-packages, not just the .dist-info metadata. What actually happens is that the site-packages copy is shadowed at import time by /app/src (via PYTHONPATH), so only its metadata is ever read. Consider rewording to something like "the installed copy is shadowed on sys.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.

**docs** [LOW] This sentence is slightly inaccurate: `uv sync` without `--no-install-project` writes a full copy of the `yunjin` modules into site-packages, not just the `.dist-info` metadata. What actually happens is that the site-packages copy is shadowed at import time by `/app/src` (via `PYTHONPATH`), so only its metadata is ever read. Consider rewording to something like "the installed copy is shadowed on `sys.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:sha256:514b02a6beb99507e88452dbd9f139c91e15978732f1b9d10d15209749f30ab6 -->
💚 Anchor the PYTHONPATH env check and bound the smoke curls
All checks were successful
/ gitleaks (pull_request) Successful in 37s
/ checks (pull_request) Successful in 2m5s
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 2m9s
/ container (pull_request) Successful in 1m41s
064d6d3cd8
The env entry match now requires exact boundaries, so PYTHONPATH=/app/src
suffix variants or similarly named variables cannot false-pass. curl
calls in the smoke step get connect/max timeouts so a hung server fails
the step instead of stalling it, and the container docs no longer imply
the installed copy is metadata-only.
Author
Collaborator

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/src and 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 after project, so [project.optional-dependencies] / [project.scripts] never match the enter rule — they hit the generic ^\[ reset rule instead, exactly as intended. The unspaced version="…" case yields an empty string, which the explicit "unable to determine version" check turns into a loud failure, per the #3960 reasoning.

### WuMing wave 9 — responses ### ✅ Fixed — unanchored PYTHONPATH env match [#3998](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3999) Fixed in commit `064d6d3`: the docs now say the installed copy's modules are shadowed at import time by `/app/src` and only its metadata is read. ### 🔴 Duplicate — PEP 440 normalisation (fourth re-raise) [#3996](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3996) Same substance as [#3958](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-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](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3995) The premise is incorrect: `^\[project\]` requires the closing bracket immediately after `project`, so `[project.optional-dependencies]` / `[project.scripts]` never match the *enter* rule — they hit the generic `^\[` reset rule instead, exactly as intended. The unspaced `version="…"` case yields an empty string, which the explicit "unable to determine version" check turns into a loud failure, per the [#3960](https://forge.marvin8.zone/marvin8/yunjin/pulls/58#issuecomment-3960) reasoning.
marvin8 approved these changes 2026-09-30 09:44:18 +00:00
marvin8 manually merged commit 6b5d76199d into main 2026-09-30 09:45:28 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
marvin8/yunjin!58
No description provided.