From 64590cb6b9a2b911ff968c88c4efe102735fee12 Mon Sep 17 00:00:00 2001 From: Nathan Esquenazi Date: Sun, 17 May 2026 17:34:46 -0700 Subject: [PATCH] harden(docker-smoke): catch !!ERROR/!!Exiting + tighten egg_info test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking observations from the review, both addressed: 1. The bad-pattern grep listed `error_exit` as a literal token, but the `error_exit()` function at docker_init.bash:5-10 only echoes the strings `"!! ERROR: "` and `"!! Exiting script (ID: $$)"` — the function name itself never appears in container logs. So `grep -E -i "error_exit"` would only fire on stray debug prints of the name, not on actual failures. The other patterns (`Failed to set (UID|GID|...)`, `groupmod: cannot`, etc.) DO catch real error_exit output, so this wasn't a coverage gap — just a dead token. Add `!! ERROR` and `!! Exiting script` to the bad-pattern set so the grep actually matches the function's output. Keep the literal `error_exit` token as belt-and-suspenders for any debug/echo of the name. 2. `test_docker_init_excludes_egg_info_during_staging` was a single `assert "egg-info" in src` check. That passes if any occurrence appears — including the explanatory comment block above the staging logic. A maintainer removing the `--exclude='*.egg-info'` from rsync but keeping the comment would slip past the test. Tighten to: - scope to the staging block (between `_stage_src=` and the `uv pip install` line) so comments outside that window can't satisfy the assertion; - require the literal `--exclude='*.egg-info'` rsync flag; - require `*.egg-info` in the block so the cp-fallback cleanup is also pinned; - additionally require `--exclude='build'`, `--exclude='dist'`, `--exclude='__pycache__'` so all four setuptools-touchable artifact dirs stay excluded. Verified: - tests/test_docker_docs_and_readonly.py — 11/11 pass. - YAML parses cleanly via `yaml.safe_load`. - Full suite: 5770 passed, 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/docker-smoke.yml | 6 +++- tests/test_docker_docs_and_readonly.py | 43 ++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/.github/workflows/docker-smoke.yml b/.github/workflows/docker-smoke.yml index 8abe273b..66de8488 100644 --- a/.github/workflows/docker-smoke.yml +++ b/.github/workflows/docker-smoke.yml @@ -195,7 +195,11 @@ jobs: # "errorless", URL paths) are improbable for these specific tokens. echo "::group::Startup log scan" LOGS="$(docker compose -p "$PROJECT" -f "$COMPOSE_FILE" logs --no-color)" - BAD_PATTERNS='EROFS|Read-only file system|Traceback|PermissionError|error_exit|groupmod: cannot|usermod: cannot|Failed to set (UID|GID|owner|permissions|ownership)' + # `!! ERROR` + `!! Exiting script` are the actual strings emitted by + # docker_init.bash's error_exit() helper — the function name itself + # never appears in output. The literal token `error_exit` is kept as + # a belt-and-suspenders catch for any stray debug/echo of the name. + BAD_PATTERNS='EROFS|Read-only file system|Traceback|PermissionError|!! ERROR|!! Exiting script|error_exit|groupmod: cannot|usermod: cannot|Failed to set (UID|GID|owner|permissions|ownership)' if echo "$LOGS" | grep -E -i "$BAD_PATTERNS"; then echo "❌ Startup logs contain known-bad pattern (see above)" exit 1 diff --git a/tests/test_docker_docs_and_readonly.py b/tests/test_docker_docs_and_readonly.py index af2ec3b1..2927199a 100644 --- a/tests/test_docker_docs_and_readonly.py +++ b/tests/test_docker_docs_and_readonly.py @@ -212,12 +212,43 @@ def test_docker_init_excludes_egg_info_during_staging(): directories. setuptools takes a different (timestamp-update) code path when one is already present in the source tree, which itself hits the :ro mount through stat/utime calls. Excluding them keeps the build - happily on the fresh-build code path.""" + happily on the fresh-build code path. + + Tight assertions on both the rsync and cp-fallback paths — a loose + `"egg-info" in src` check would pass on a stray comment mention, so + we require the actual exclusion mechanics to be present. + """ src = (REPO / "docker_init.bash").read_text(encoding="utf-8") - # Either rsync --exclude= form or the cp-fallback's explicit rm -rf — - # accept either provided the egg-info exclusion exists. - assert "egg-info" in src, ( - "docker_init.bash staging must exclude *.egg-info from the copy " - "to avoid setuptools' timestamp-update code path." + + # Find the staging block: rsync invocation OR cp-fallback. Both must + # actually exclude *.egg-info — a comment mention is not enough. + stage_idx = src.index("_stage_src=") + install_idx = src.index("uv pip install", stage_idx) + stage_block = src[stage_idx:install_idx] + + # Rsync path must carry --exclude='*.egg-info'. + assert "--exclude='*.egg-info'" in stage_block, ( + "docker_init.bash rsync invocation must include " + "--exclude='*.egg-info' so setuptools' timestamp-update code path " + "doesn't fire (which itself hits the :ro mount through stat/utime)." + ) + + # cp-fallback path must explicitly rm the egg-info dir after copy + # (cp -a has no --exclude flag, so the cleanup happens post-copy). + assert "*.egg-info" in stage_block, ( + "docker_init.bash cp-fallback must remove $_stage_src/*.egg-info " + "after copy so the install runs on the fresh-build code path." + ) + + # Both build and dist must also be excluded — setuptools touches them + # under different conditions but the failure mode is identical. + assert "--exclude='build'" in stage_block, ( + "rsync staging must --exclude='build' (setuptools build artifacts)." + ) + assert "--exclude='dist'" in stage_block, ( + "rsync staging must --exclude='dist' (setuptools build artifacts)." + ) + assert "--exclude='__pycache__'" in stage_block, ( + "rsync staging must --exclude='__pycache__' to keep the copy minimal." )