(MOT-3889) fix(scrapling): ship as python source bundle instead of deploy: image - #433
Merged
Merged
Conversation
scrapling/v0.2.0 dropped its Dockerfile for runtime.base_image + scripts.install, but deploy: image still fired the container-build job, which died on `open Dockerfile: no such file or directory` (run 28822396424). Switch scrapling to deploy: bundle and teach the pipeline about python bundles: - _bundle.yml: python path stages the worker source (src/ + pyproject.toml + iii.worker.yaml); scripts.start self-bootstraps since the engine's bundle validator executes only scripts.start - _publish-registry.yml: boot bundles via the manifest's scripts.start instead of hardcoded `node ./index.mjs`; widen the interface-collection wait to 600s for self-bootstrapping bundles - validate_worker.py: mirror the engine's bundle-manifest rules at PR time (no scripts.install/setup, non-empty scripts.start, runtime.base_image must name an engine-preset ref) - scrapling/iii.worker.yaml: deploy: bundle, runtime.base_image pinned to the engine-preset docker.io/iiidev/python:latest (runtime.kind is deprecated), dependency bootstrap folded into scripts.start Requires the engine change accepting preset refs for bundle runtime.base_image (iii repo, bundle_download.rs). Fixes MOT-3889
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 35 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughThis PR adds ChangesBundle Deploy Pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Bundle as "_bundle.yml"
participant Registry as "_publish-registry.yml"
participant Worker as "Bundled Worker Process"
CI->>Bundle: Trigger bundle build
Bundle->>Bundle: Stage worker source (exclude tests/git/README)
Bundle->>Bundle: Verify staged artefact (pyproject.toml/index.mjs + iii.worker.yaml)
Bundle->>Bundle: Tar staged directory
CI->>Registry: Publish bundle
Registry->>Registry: Parse iii.worker.yaml for scripts.start
Registry->>Worker: sh -c "$start_cmd"
Registry->>Worker: Wait up to 600s for interface registration
Worker-->>Registry: Interfaces collected
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/_bundle.yml (2)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: no longer just "entrypoint + iii.worker.yaml".
Since the tarball now packs the entire
$STAGE_DIR(including full Python source trees), the comment describing the archive contents as "the staged entrypoint +iii.worker.yaml" is inaccurate for Python bundles.✏️ Suggested comment update
- # straight to the staged files, not at `$STAGE_DIR`. The resulting archive expands - # straight to the staged entrypoint + `iii.worker.yaml`. + # `tar -C` so the archive contents are rooted at the staged + # files, not at `$STAGE_DIR`. The resulting archive expands to + # everything staged for the worker (entrypoint/source tree + `iii.worker.yaml`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/_bundle.yml around lines 172 - 175, The tarball comment in the bundle workflow is stale and still says the archive contains only the staged entrypoint plus iii.worker.yaml, but the bundle now includes the full contents of $STAGE_DIR. Update the comment near the tar -C step to accurately describe that the archive is rooted at the staged files and now packages the entire staged bundle, including Python source trees.
117-136: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDeny-list staging is more fragile than the Node path's allow-list.
Unlike Node staging (which copies only
index.mjs+iii.worker.yaml), Python staging rsyncs the entire worker directory excepttests,.git*, andREADME.md. Any future stray artifact (e.g.,.venv/,__pycache__/, editor configs, or an accidentally-committed.env) would ship into the release tarball since nothing else is excluded.♻️ Suggested additional excludes
rsync -a --exclude tests --exclude '.git*' --exclude README.md \ + --exclude '__pycache__' --exclude '*.egg-info' --exclude '.venv' \ + --exclude '.env*' --exclude '.mypy_cache' --exclude '.ruff_cache' \ "$WORKER/" "$stage/"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/_bundle.yml around lines 117 - 136, The Python staging step is too permissive because the rsync-based deny-list only excludes a few paths, so unintended files can slip into the bundle. Update the Stage Python artefact logic in the workflow to use a stricter allow-list or expand the rsync excludes so only the required worker sources and manifest are staged, matching the tighter Node staging approach. Make the fix in the Stage Python artefact step that uses the WORKER, stage, and rsync command..github/scripts/validate_worker.py (1)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
BUNDLE_PRESET_IMAGESin sync with the engine catalog
BUNDLE_PRESET_IMAGESduplicates the engine’s preset list, so changes insandbox_daemon/catalog.rscan silently desync validation here. Pull this from the same source of truth or add a sync check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/validate_worker.py around lines 55 - 63, BUNDLE_PRESET_IMAGES is duplicating the engine’s preset catalog, so validation can drift from sandbox_daemon/catalog.rs. Update validate_worker.py to derive the allowed base image set from the same source of truth used by the engine, or add a startup/sync check that compares BUNDLE_PRESET_IMAGES against the engine presets so mismatches are caught automatically; use the BUNDLE_PRESET_IMAGES constant and the bundle validator/catalog preset definitions as the key anchors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/scripts/validate_worker.py:
- Around line 55-63: BUNDLE_PRESET_IMAGES is duplicating the engine’s preset
catalog, so validation can drift from sandbox_daemon/catalog.rs. Update
validate_worker.py to derive the allowed base image set from the same source of
truth used by the engine, or add a startup/sync check that compares
BUNDLE_PRESET_IMAGES against the engine presets so mismatches are caught
automatically; use the BUNDLE_PRESET_IMAGES constant and the bundle
validator/catalog preset definitions as the key anchors.
In @.github/workflows/_bundle.yml:
- Around line 172-175: The tarball comment in the bundle workflow is stale and
still says the archive contains only the staged entrypoint plus iii.worker.yaml,
but the bundle now includes the full contents of $STAGE_DIR. Update the comment
near the tar -C step to accurately describe that the archive is rooted at the
staged files and now packages the entire staged bundle, including Python source
trees.
- Around line 117-136: The Python staging step is too permissive because the
rsync-based deny-list only excludes a few paths, so unintended files can slip
into the bundle. Update the Stage Python artefact logic in the workflow to use a
stricter allow-list or expand the rsync excludes so only the required worker
sources and manifest are staged, matching the tighter Node staging approach.
Make the fix in the Stage Python artefact step that uses the WORKER, stage, and
rsync command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88e1ee98-e414-440c-b013-0cf8b51f1a81
📒 Files selected for processing (4)
.github/scripts/validate_worker.py.github/workflows/_bundle.yml.github/workflows/_publish-registry.ymlscrapling/iii.worker.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Release run for
scrapling/v0.2.0failed: https://github.com/iii-hq/workers/actions/runs/28822396424deploy: imagefires the container-build job, which runs buildx againstscrapling/Dockerfile— but v0.2.0 moved scrapling toruntime.base_image+scripts.installand deleted the Dockerfile, so buildx dies withopen Dockerfile: no such file or directory.What
Ship scrapling as
deploy: bundleand teach the pipeline about python bundles:_bundle.yml— python path implemented: stages the worker source (src/+pyproject.toml+iii.worker.yaml, minus tests/README) as the bundle. The engine's bundle validator executes onlyscripts.start(rejectsscripts.install/setup), so the start command self-bootstraps; native deps + browsers can't be vendored per-arch into one archive anyway. Verify step is language-aware; pack step tars the stage dir generically._publish-registry.yml— bundle smoke-boot readsscripts.startfrom the extracted manifest instead of hardcodingnode ./index.mjs, so it exercises the exact command the engine runs (works for node and python bundles alike). Interface-collection wait widened to 600s for bundles since they may pip-install at boot.validate_worker.py— PR-time mirror of the engine's bundle-manifest rules: noscripts.setup/install, non-emptyscripts.start, andruntime.base_imagemust name an engine-preset ref verbatim. Passes scrapling/claude-code/pi/opencode; rejects synthetic bad manifests.scrapling/iii.worker.yaml—deploy: bundle;runtime.base_image: docker.io/iiidev/python:latest(the documented field —runtime.kindis deprecated, and empty kind silently defaults to the node rootfs); dependency bootstrap (pip install -e . && scrapling install) folded intoscripts.start.Depends on
An engine-side change in the iii repo (uncommitted yet):
bundle_download.rsnow acceptsruntime.base_imagefor bundles when it names an engine-preset ref verbatim (docker.io/iiidev/python:latest/iiidev/node:latest), still rejecting arbitrary refs. Without it,iii worker addof the published scrapling bundle is rejected at install time. CI in this repo does not exercise that path, so this PR is green independently.After merge
The
scrapling/v0.2.0tag still points at thedeploy: imagecommit — re-run the release viaworkflow_dispatch(builds from main's tree) or cutv0.2.1.Fixes MOT-3889
Summary by CodeRabbit
New Features
Bug Fixes