Skip to content

Build the Docker image in CI, and pin claude.yml to the stable channel - #13

Merged
jehrr merged 2 commits into
mainfrom
fix/ci-docker-and-claude-channel
Sep 17, 2026
Merged

jehrr merged 2 commits into
mainfrom
fix/ci-docker-and-claude-channel

Conversation

@jehrr

@jehrr jehrr commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Two CI gaps, both of the same shape: a check that exists elsewhere in
the family and never arrived here.

No job built the Docker image. That is how three repos in this family
shipped an image that died with ModuleNotFoundError on every invocation,
--help included: the Dockerfile COPYs an explicit list -- correct, the
image should carry no test suite and no stray .env -- and the list falls
behind the imports with nothing to notice. The job is four steps, each
for a distinct failure: the image builds; its entrypoint runs, which is
the invocation a missing module breaks; Chromium actually launches,
rather than the playwright install --with-deps layer merely exiting 0;
and the image contains no .env, no test suite and no fixtures, because a
.env baked into an image is a credential published to everyone who can
pull it.

The COPY list was verified against playwright_scraper.py's transitive
local import graph before writing the job, and is complete. The job
itself could not be run locally -- the docker daemon is not reachable
from this account -- so its first real proof is this PR's own CI, which
is the point of adding it.

claude.yml let the action install latest. On 2026-09-08 that release
installed no binary where the action looks, so every run died with
"Claude Code native binary not found at ~/.local/bin/claude". The fix --
install the stable channel explicitly and pass
path_to_claude_code_executable, with the path exported through
GITHUB_ENV because ${{ env.HOME }} is empty in the workflow env context
and the action then silently falls back to latest -- was already in
claude-code-review.yml in this very repo and had never reached its twin.
Pinning the CHANNEL rather than a version means a fixed upstream release
needs no edit here.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

🤖 Generated with Claude Code

Two CI gaps, both of the same shape: a check that exists elsewhere in
the family and never arrived here.

No job built the Docker image. That is how three repos in this family
shipped an image that died with ModuleNotFoundError on every invocation,
--help included: the Dockerfile COPYs an explicit list -- correct, the
image should carry no test suite and no stray .env -- and the list falls
behind the imports with nothing to notice. The job is four steps, each
for a distinct failure: the image builds; its entrypoint runs, which is
the invocation a missing module breaks; Chromium actually launches,
rather than the `playwright install --with-deps` layer merely exiting 0;
and the image contains no .env, no test suite and no fixtures, because a
.env baked into an image is a credential published to everyone who can
pull it.

The COPY list was verified against playwright_scraper.py's transitive
local import graph before writing the job, and is complete. The job
itself could not be run locally -- the docker daemon is not reachable
from this account -- so its first real proof is this PR's own CI, which
is the point of adding it.

claude.yml let the action install `latest`. On 2026-09-08 that release
installed no binary where the action looks, so every run died with
"Claude Code native binary not found at ~/.local/bin/claude". The fix --
install the stable channel explicitly and pass
path_to_claude_code_executable, with the path exported through
GITHUB_ENV because ${{ env.HOME }} is empty in the workflow env context
and the action then silently falls back to latest -- was already in
claude-code-review.yml in this very repo and had never reached its twin.
Pinning the CHANNEL rather than a version means a fixed upstream release
needs no edit here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines +55 to +65
- name: Install Claude Code (stable channel)
if: env.HAS_CLAUDE_TOKEN == 'true'
run: |
curl -fsSL https://claude.ai/install.sh | bash -s stable
"$HOME/.local/bin/claude" --version
# Through GITHUB_ENV, not `${{ env.HOME }}`: HOME is a runner
# variable and is not in the workflow `env` context, so the
# expression form expands to an empty string and the action falls
# back to installing `latest` — the very thing this step exists to
# avoid, and silently.
echo "CLAUDE_BIN=$HOME/.local/bin/claude" >> "$GITHUB_ENV"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Altitude / reuse: this duplicates claude-code-review.yml verbatim, reproducing the exact drift risk this PR's own CHANGELOG describes.

This "Install Claude Code (stable channel)" block (and the matching path_to_claude_code_executable: ${{ env.CLAUDE_BIN }} line below) is a line-for-line copy of the block already in .github/workflows/claude-code-review.yml. The CHANGELOG entry for this PR even says so explicitly: "The fix had already landed in claude-code-review.yml in this repo and never reached its twin."

Copy-pasting the fix into the second file doesn't remove the underlying problem — it just resets the clock on it. There's no .github/actions/ composite action in this repo, so the next time this install step needs to change (upstream deprecates stable, the install path changes, etc.), whoever edits one workflow can just as easily forget the other again, exactly as happened here.

Suggest factoring this into a composite action (e.g. .github/actions/install-claude-stable/action.yml) that both claude.yml and claude-code-review.yml call, so there is exactly one place to fix next time instead of two files that can silently drift apart.

§17's check #1, which this repo did not have. It exists because of a
defect no other check in a suite like this can see: a sibling shipped
`classify(html, url=...)` in two of three engines against a callee whose
second parameter is `status`, and BOTH crashed on their FIRST fetch --
invisible to import, --help, compileall, the undefined-name walk and
several hundred green assertions, because none of those calls a function
the way a live run does.

Two failure modes, and the second is the one a weaker version swallows:
a call whose arguments do not fit the signature, and a call to a name
the shared module DOES NOT DEFINE. bbb-scraper resolved the callee with
getattr(..., None) and skipped whatever came back not-callable, so three
calls into an API that did not exist sat under a green run of its own
binding check. Absent is the loudest failure available, not "nothing to
bind", and it is reported here as a failure naming the caller and line.

Conservative by construction: a call using *args or **kwargs is skipped
rather than guessed at, and a name bound anywhere in the calling file
shadows a same-named module -- an engine takes `proxy_pool` as a
PARAMETER, and `proxy_pool.next()` on that parameter is a method call,
not a module attribute. Without that rule the check reports twenty-one
false positives on a clean repo. It also asserts that it found calls to
bind at all, so it cannot pass by scanning nothing.

Run across all eighteen repos of this family before being added to any
of them: one real defect, in bbb-scraper, now fixed there.

Verified by control -- adding a finish_run() call with thirteen
positional arguments turns the suite red with "too many positional
arguments", and removing it turns it green. The first version of this
check crashed on that control instead of reporting it: it passed three
arguments to a two-argument check(), which is precisely the defect class
it exists to catch, found by its own control rather than by review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# ModuleNotFoundError on every invocation, `--help` included: the
# Dockerfile COPYs an explicit list (right -- the image should carry no
# test suite and no stray .env) and the list fell behind the imports.
# Four steps, each for a distinct failure. Under a minute.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"Under a minute" reads as a measured number but nothing here measures it, and the job has no docker layer cache or actions/cache for pip/playwright (no cache-from, no actions/cache, no BuildKit cache mount). A cold docker build that runs pip install plus playwright install --with-deps chromium — apt-installing browser dependencies and downloading the Chromium binary — routinely takes well over a minute on a fresh ubuntu-latest runner, before the three docker run steps that follow. Either cite an actual observed runtime (and note it's cold-cache) or drop the specific figure so the comment doesn't read as a benchmarked fact.

Comment thread smoke_test.py
Comment on lines +1443 to +1446
callers = [e + ".py" for e in ENGINES]
callers += [n for n in ("page_flow.py", "product_parser.py",
"scraper_api_client.py")
if os.path.exists(os.path.join(REPO_ROOT, n))]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

callers omits fingerprint_client.py, even though it's listed in shared (line 1436) and does make real calls into another shared module — env_config.load_env() and env_config.env_value("TWOCAPTCHA_KEY") at fingerprint_client.py:348-349. Those calls are structurally unreachable by this test's ast.walk loop.

That undercuts the claim this check makes in CHANGELOG.md (line 15): "binds every call into a shared module against the callee's real signature." It's not every call — calls made from one shared module into another are skipped entirely, and this is exactly the file/call-path (fingerprint_client.py → env_config) that CHANGELOG's [0.1.3] entry says had a real prior defect. If env_config.env_value's signature ever changes in a way that breaks fingerprint_client.py's call, this test stays green while a live run crashes on first use.

Suggest adding "fingerprint_client.py" to callers.

Comment thread smoke_test.py
Comment on lines +1476 to +1477
if isinstance(node.func, ast.Name) and node.func.id in direct:
target = direct[node.func.id]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor asymmetry: this direct-import branch resolves a call by bare name alone, with no check against bound. The ast.Attribute branch just below (lines 1478-1482) explicitly excludes names in bound so a locally shadowed module reference isn't mistaken for the real module — this branch has no equivalent guard. If a caller ever reassigns a directly-imported name (e.g. parse_products = some_local_wrapper then calls parse_products(...)), this would bind the call against product_parser.parse_products's real signature instead of the local rebinding, producing a false pass/fail unrelated to what's actually being called at runtime.

Not triggered by any current caller, but worth the same and node.func.id not in bound guard for consistency with the deliberately-conservative design described in the docstring above.

@jehrr
jehrr merged commit 80ab804 into main Sep 17, 2026
13 checks passed
@jehrr
jehrr deleted the fix/ci-docker-and-claude-channel branch September 17, 2026 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant