refactor(builder-sidecar): ephemeral builder containers launched from host#162
Open
refactor(builder-sidecar): ephemeral builder containers launched from host#162
Conversation
azchin
commented
Apr 1, 2026
| repo_hash = self.__get_repo_hash() | ||
| return f"{self.name}:{repo_hash}-{sanitizer}-snapshot" | ||
|
|
||
| def _snapshot_lock_path(self, snapshot_tag: str) -> Path: |
Collaborator
Author
There was a problem hiding this comment.
Do we need to restore this locking logic?
| cached_hash = cache_file.read_text().strip() | ||
| return cached_hash == base_image_hash and self._snapshot_exists(snapshot_tag) | ||
|
|
||
| def create_snapshot( |
Collaborator
Author
There was a problem hiding this comment.
In general, seems odd we're completely removing snapshot logic from this module.
azchin
commented
Apr 3, 2026
Collaborator
Author
There was a problem hiding this comment.
@acorn421 Can you check out these verify-patch-* scripts and make something similar using CRSBench for E2E testing?
azchin
commented
Apr 3, 2026
Collaborator
Author
There was a problem hiding this comment.
@acorn421 Testing logic here for rebuild sidecar
Builder-sidecar: FastAPI server that spawns ephemeral Docker containers for builds and tests via Docker socket. Extracts build CMD from image via docker inspect, disables REPLAY_ENABLED, uses oss_crs_handler.sh for both build (image CMD) and test (test.sh). Runner-sidecar: FastAPI server on base-runner for POV reproduction via OSS-Fuzz reproduce script. Simplify oss_crs_handler.sh to: git apply + run $OSS_CRS_BUILD_CMD. Signed-off-by: Andrew Chin <achin34@gatech.edu>
…-test - apply-patch-build: submit patch to builder-sidecar, poll for result - apply-patch-test: renamed from run-test, takes patch_path explicitly - run-pov: submit POV to runner-sidecar - download-build-output: fetch versioned artifacts as zip - Remove _last_patch_path stateful hack (broken across CLI invocations) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…builder - Sidecar injection triggered by any CRS with snapshot builds - Remove _builder_crs property and "at most one builder" restriction - Remove is_builder gates from LLM, bug-finding, and compose template - Fix BASE_IMAGE to use snapshot image instead of raw target - Add get_rebuild_out_dir to workdir for sidecar artifact storage Signed-off-by: Andrew Chin <achin34@gatech.edu>
- Unit tests for builder-sidecar, runner-sidecar, and libCRS sidecar methods - Integration tests for sidecar lifecycle - sandbox/crs-sidecar-test: manual e2e test CRS with ground-truth patch - Remove old builder/ directory (replaced by builder-sidecar) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…g properties - Remove snapshot: bool from BuildConfig - Remove run_snapshot: bool from CRSRunPhaseModule - Update validate_dockerfile_requirement: all modules always require dockerfile - Remove snapshot_builds, has_snapshot, has_builder_module properties from CRSConfig - Delete test_snapshot_flag, test_snapshot_module_can_omit_dockerfile tests - Add test_module_requires_dockerfile_always test Signed-off-by: Andrew Chin <achin34@gatech.edu>
- Remove _any_needs_snapshot property from CRSCompose - Remove snapshot creation block from CRSCompose.build_target() - Remove snapshot image check and snapshot_image_tag assignment from CRSCompose.run() - Replace non_snapshot_builds filter with plain builds in CRS.build_target() and is_target_built() - Remove snapshot_image_tag attr, create_snapshot(), get_snapshot_image_name(), _snapshot_lock_path(), _snapshot_exists(), _is_snapshot_reusable(), _get_base_image_hash() from Target - Remove tempfile import (no longer needed) - Update renderer.py: disable sidecar injection (has_sidecar=False), remove snapshot_image_tag from target reference, remove run_snapshot logic from module env computation - Update Jinja2 template: remove run_snapshot conditional blocks - Delete test_crs_compose_snapshot_sanitizer.py and test_target_snapshot.py Signed-off-by: Andrew Chin <achin34@gatech.edu>
- Remove has_sidecar conditional; sidecars now always injected as infra - Compute rebuild_out_dir unconditionally from crs_list[0] - Remove snapshot_image_tag, has_sidecar, sidecar_crs_name from context - Template: replace single sidecar_crs_name alias with crs_list loop - Template: BASE_IMAGE uses target.get_docker_image_name() unconditionally - Sandbox crs.yaml: remove inc-builder snapshot build entry Signed-off-by: Andrew Chin <achin34@gatech.edu>
- Add --incremental-build store_true arg to build-target subcommand - Add --incremental-build store_true arg to run subcommand - Pass incremental_build=args.incremental_build in both CLI dispatch sites - Add incremental_build: bool = False to CRSCompose.build_target() signature - Add incremental_build: bool = False to CRSCompose.run() signature - Thread incremental_build through __run(), __run_local(), __prepare_local_running_env() - 4 unit tests verify flag parsing on both subcommands (present/absent) Signed-off-by: Andrew Chin <achin34@gatech.edu>
- get_test_image() signature drops builder_name parameter per D-05
- run_ephemeral_test() snapshot_tag and commit tag use test-{build_id}
- server.py _handle_test() updated to call get_test_image() without builder_name
- All sidecar tests updated to assert test-{build_id} format (no builder_name)
Signed-off-by: Andrew Chin <achin34@gatech.edu>
- Add docker/requests.exceptions imports to crs_compose.py
- _snapshot_one(): helper to run a container from base_image and commit on success
- _create_incremental_snapshots(): commit builder snapshots (build-{name}-{build_id}) and
project image snapshot (test-{build_id}) with all-or-nothing cleanup on failure (D-04/D-05/D-06)
- _check_snapshots_exist(): pre-run validation returning clear error with oss-crs build-target
--incremental-build instructions when any snapshot is missing (SNAP-05/D-09)
- Wire _create_incremental_snapshots into build_target() after success, gated by incremental_build
- Wire _check_snapshots_exist into run() before compose starts, gated by incremental_build
- 9 unit tests passing with mocked Docker SDK
Signed-off-by: Andrew Chin <achin34@gatech.edu>
- _handle_test() now reads PROJECT_BASE_IMAGE env var directly (not BASE_IMAGE_*) - Raises ValueError if PROJECT_BASE_IMAGE is not set (clear error message) - Template unconditionally emits PROJECT_BASE_IMAGE on builder-sidecar service - Module docstring documents the new env var Signed-off-by: Andrew Chin <achin34@gatech.edu>
…ts on --incremental-build flag - Bundle oss_crs_handler.sh in sidecar image and bind-mount into ephemeral build/test containers via HANDLER_SCRIPT_PATH env var - Sidecar only uses snapshot images when INCREMENTAL_BUILD=true (fresh base image otherwise), enforcing SNAP-04 - Forward OSS-Fuzz env vars (FUZZING_ENGINE, SANITIZER, ARCHITECTURE, FUZZING_LANGUAGE) from sidecar to ephemeral containers - Stream ephemeral container logs to sidecar stdout with [build:name]/[test:id] prefix while still capturing for response - Extract OSS_FUZZ_TARGET_ENV constant in env_policy.py to dedup env var definitions Signed-off-by: Andrew Chin <achin34@gatech.edu>
…rce limits
- Replace v1/v2 versioning with rebuild_id (auto-incrementing int, optional
override). Output dir: /rebuild_out/{crs_name}/{rebuild_id}/
- Merge download_rebuild_output into download_build_output: with rebuild_id
fetches from sidecar, without uses local filesystem. Ephemeral containers
auto-try sidecar via OSS_CRS_REBUILD_ID env var, fallback to local.
- Rename build_id to rebuild_id on sidecar endpoints; add crs_name form field
- Forward CRS resource limits (cpuset, mem_limit) per-request to ephemeral containers
- Replace sequential job queue with ThreadPoolExecutor (MAX_PARALLEL_JOBS=4)
- Remove dead include_snapshot code from env_policy and renderer
Signed-off-by: Andrew Chin <achin34@gatech.edu>
…ardized response files - builder defaults to BUILDER_MODULE env var; run_pov defaults to runner-sidecar - builder_name param separates service routing from image resolution - submit_build_output validates OSS_CRS_CURRENT_PHASE: local at build-target, sidecar upload at run - response dir files standardized: retcode, stdout.log, stderr.log, rebuild_id - run_pov takes rebuild_id (int) instead of build_id (str) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…_output_dir - POST /upload-build-output on builder sidecar for run-phase artifact pre-staging - rebuild_output_dir() extracted as shared path helper - runner-sidecar: build_id -> rebuild_id, stdout added to POVResult Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
…OSS_CRS_PROJ_PATH/test.sh) - New run_tests.sh script resolves test script with fallback chain - Mounted into both sidecar ephemeral containers and orchestrator snapshot containers - Replaces hardcoded test.sh references in docker_ops.py and crs_compose.py - Test snapshot is optional (skip if no test script found) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…ecified Signed-off-by: Andrew Chin <achin34@gatech.edu>
…{crs_name}/{rebuild_id}/
- Replace /out bind mount with container.get_archive() to preserve snapshot
state + new files (bind mount was shadowing snapshot's prebuilt /out)
- Runner sidecar takes crs_name, resolves harness at /out/{crs_name}/{rebuild_id}/
- libCRS run_pov sends crs_name to runner
Signed-off-by: Andrew Chin <achin34@gatech.edu>
…compile) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…(OOM on compile)" This reverts commit 1f5a6bd0ee7f10b973f54c63806ed0c3a1660cb5. Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
…al containers - Orchestrator bakes additional_env (e.g. SANITIZER=coverage) and CMD into snapshot via docker commit --changes - Sidecar extracts image env for OSS-Fuzz keys, image env takes precedence over sidecar env (so coverage snapshot keeps SANITIZER=coverage) Signed-off-by: Andrew Chin <achin34@gatech.edu>
Replace docker cp artifact extraction with bind-mounted OSS_CRS_BUILD_OUT_DIR so ephemeral containers use submit-build-output (same as build-target phase). Key changes: - Mount rebuild_out_dir into ephemeral build containers at /OSS_CRS_BUILD_OUT_DIR - Write patch file to shared volume (not tmpdir) since sidecar tmpdir is invisible to Docker daemon in Docker-in-Docker via socket mount - Add HOST_REBUILD_OUT_DIR env to sidecar for host path resolution - Mount rebuild_out_dir into CRS patcher containers at /OSS_CRS_REBUILD_OUT_DIR so download_build_output uses local filesystem copy instead of HTTP+zip - Remove _download_from_sidecar HTTP/zip endpoint usage from libCRS - Add OSS_CRS_NAME and OSS_CRS_RUN_ENV_TYPE to ephemeral container env - Thread crs_name through /test endpoint to run_ephemeral_test - Update runner to resolve harness under build/ subdirectory - Change runner volume mount from :ro to :rw (reproduce script needs it) Signed-off-by: Andrew Chin <achin34@gatech.edu>
…ests E2E tests using mock-c fixtures exercising the full sidecar pipeline: - builder-sidecar-full: coverage builds, coverage rebuilds, test.sh, POV - builder-sidecar-lite: patch verify (POV crash → rebuild → no crash → tests pass) Both use --diff CLI to pass patches via FETCH_DIR (no baked-in patches). Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
…observability
- Extract _run_ephemeral() shared by run_ephemeral_build and run_ephemeral_test
- Fix run_ephemeral_test using sidecar tmpdir for patch (same bug as build)
- Make oss_crs_handler.sh fail early on missing patch file, add --verbose to git apply
- verify_patch: reproduce all POVs, add timing logs to OSS_CRS_LOG_DIR
- verify-patch-all.py: run across CRSBench benchmarks with timing and log paths
- verify-patch-{inc,no-inc}.sh: accept args, extract timing via oss-crs artifacts
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Snapshots are now tagged with a content-hash key derived from the builder
image ID (oss-crs-snapshot:content-{hash}). If the builder image hasn't
changed between build_ids, the existing snapshot is reused via docker tag
instead of recompiling.
A file lock (/tmp/oss-crs-snapshot-locks/) serializes concurrent snapshot
creation for the same builder image, preventing duplicate work when
multiple build-target processes run in parallel.
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Remove snapshot commit logic from _run_ephemeral — snapshots are created by _create_incremental_snapshots during build-target, not by ephemeral rebuild/test containers. This was causing snapshot accumulation even without --incremental-build. Also: add OSS_CRS_PROJ_PATH env to ephemeral containers so run_tests.sh can find test.sh, move verify-patch scripts to scripts/ with embedded compose config. Signed-off-by: Andrew Chin <achin34@gatech.edu>
Replace Docker image ID-based dedup (which changes every rebuild) with
target repo hash-based dedup. The content key is derived from the target
image name ({name}:{repo_hash}), which is deterministic across runs for
the same source state. This prevents accumulation of content-* snapshot
images on repeated build-target invocations.
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Move ruff check, ruff format, pyright, and unit tests into a single verify script runnable via `uv run verify`. Move ruff and pyright from project dependencies to dev dependency group. Signed-off-by: Andrew Chin <achin34@gatech.edu>
Remove unused imports and variables, reformat 22 files, fix base class signature for download_build_output to include rebuild_id, and suppress E402 in test_builder_sidecar where sys.path setup requires import reordering. Signed-off-by: Andrew Chin <achin34@gatech.edu>
Sidecar writes stdout.log, stderr.log, exit_code to rebuild_out_dir/
{crs_name}/{rebuild_id}/ after each ephemeral run. libCRS reads logs
from the shared OSS_CRS_REBUILD_OUT_DIR mount instead of HTTP response,
enabling postmortem analysis from the host.
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Align test call sites with new signatures for run_ephemeral_test, run_ephemeral_build, _handle_test, download_build_output, and the /run-pov endpoint. Replace removed _get_image_id references with current snapshot internals. Set OSS_CRS_REBUILD_OUT_DIR and create log files on disk for apply_patch_build/test tests. Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
azchin
changed the title
Signed-off-by: Andrew Chin <achin34@gatech.edu>
Signed-off-by: Andrew Chin <achin34@gatech.edu>
There was a problem hiding this comment.
Pull request overview
Refactors the builder/runner sidecar architecture to use framework-injected, host-launched ephemeral containers (optionally backed by operator-controlled snapshot images via --incremental-build), and updates libCRS + compose generation + tests/docs accordingly.
Changes:
- Injects new FastAPI-based builder-sidecar and runner-sidecar services into the run-phase docker-compose template, with a shared rebuild artifact directory.
- Adds
--incremental-buildtooss-crs build-targetandoss-crs run, plus snapshot creation/validation logic inCRSCompose. - Updates libCRS contracts/CLI/docs (rename
run-test→apply-patch-test,build_id→rebuild_id, response fields) and refreshes unit/integration tests.
Reviewed changes
Copilot reviewed 71 out of 77 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds dev dependencies required for sidecar servers/tests (FastAPI, uvicorn, httpx, python-multipart, etc.). |
| scripts/verify.py | New repo-level “verify” script to run ruff/pyright/unit tests. |
| scripts/verify-patch-no-inc.sh | New helper script to run a patch-verify flow without incremental snapshots. |
| scripts/verify-patch-inc.sh | New helper script to run a patch-verify flow with --incremental-build. |
| scripts/verify-patch-all.py | New benchmark runner for patch verification across multiple CRSBench projects. |
| scripts/init.py | Makes scripts importable as a package for the new verify entrypoint. |
| registry/crs-codex.yaml | Updates the referenced crs-codex ref. |
| pyproject.toml | Moves lint/type tools to dev group and adds verify console script + sidecar dev deps. |
| oss-crs-infra/runner-sidecar/server.py | New FastAPI runner-sidecar service implementing /run-pov. |
| oss-crs-infra/runner-sidecar/requirements.txt | Runner-sidecar Python requirements. |
| oss-crs-infra/runner-sidecar/Dockerfile | Container image for runner-sidecar (based on OSS-Fuzz base-runner). |
| oss-crs-infra/builder-sidecar/server.py | New FastAPI builder-sidecar service orchestrating ephemeral build/test containers. |
| oss-crs-infra/builder-sidecar/run_tests.sh | Script to resolve and run project tests with OSS-Fuzz conventions/fallbacks. |
| oss-crs-infra/builder-sidecar/requirements.txt | Builder-sidecar Python requirements (FastAPI + docker SDK). |
| oss-crs-infra/builder-sidecar/oss_crs_handler.sh | Patch-apply + build handler run inside ephemeral builder containers. |
| oss-crs-infra/builder-sidecar/Dockerfile | Container image for builder-sidecar service. |
| oss-crs-infra/builder-sidecar/docker_ops.py | Docker SDK lifecycle for ephemeral build/test containers + snapshot selection. |
| oss_crs/tests/unit/test_target_snapshot.py | Removes legacy target snapshot unit tests. |
| oss_crs/tests/unit/test_runner_sidecar.py | Adds unit tests for runner-sidecar behavior (mocked subprocess/fs). |
| oss_crs/tests/unit/test_renderer_run_compose.py | Updates compose rendering tests for always-injected sidecars + env var emission. |
| oss_crs/tests/unit/test_incremental_snapshots.py | Adds unit tests for incremental snapshot creation/validation logic. |
| oss_crs/tests/unit/test_env_policy.py | Updates env-policy tests to remove snapshot env and assert BUILDER_MODULE injection. |
| oss_crs/tests/unit/test_crs_compose_snapshot_sanitizer.py | Removes legacy snapshot sanitizer precedence tests. |
| oss_crs/tests/unit/test_cli_incremental_build.py | Adds unit tests for CLI parsing of --incremental-build. |
| oss_crs/tests/unit/config/test_crs.py | Updates config validation tests for removal of snapshot/run_snapshot fields. |
| oss_crs/tests/integration/test_sidecar_lifecycle.py | Adds integration tests for ephemeral container lifecycle and snapshot reuse. |
| oss_crs/tests/integration/test_resource_limits.py | Adjusts integration test timing to be less flaky. |
| oss_crs/tests/integration/test_builder_sidecar_lite.py | Adds “lite” E2E flow test for patch verification via sidecars. |
| oss_crs/tests/integration/test_builder_sidecar_full.py | Adds “full” E2E flow test (multi-builder + coverage + POV + test). |
| oss_crs/tests/integration/fixtures/mock-c-patches/patch_0.diff | Adds fixture patch used by integration/E2E tests. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/oss-crs/patcher.Dockerfile | Adds patcher container for lite E2E. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/oss-crs/docker-bake.hcl | Adds bake file (intentionally empty) for lite fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/oss-crs/crs.yaml | Adds lite CRS config fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/oss-crs/builder.Dockerfile | Adds builder Dockerfile fixture for lite. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/bin/verify_patch | Adds patch verification script used in lite E2E. |
| oss_crs/tests/integration/fixtures/builder-sidecar-lite/bin/compile_target | Adds compile+submit script used in lite builder image. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/test-e2e.sh | Adds shell-based E2E test script for full fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/test-e2e.py | Adds Python-API E2E test script for full fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/test-e2e-all.sh | Runs both shell + Python E2E scripts sequentially. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/patcher.Dockerfile | Adds patcher container for full E2E. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/example-compose.yaml | Adds example compose doc fixture (now sidecar-oriented). |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/docker-bake.hcl | Adds bake file (intentionally empty) for full fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/crs.yaml | Adds full CRS config fixture (multi-builder). |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/coverage-builder.Dockerfile | Adds coverage builder Dockerfile fixture. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/oss-crs/builder.Dockerfile | Adds default builder Dockerfile fixture for full. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/bin/compile_target_coverage | Adds coverage compile+submit script for full. |
| oss_crs/tests/integration/fixtures/builder-sidecar-full/bin/compile_target | Adds compile+submit script for full. |
| oss_crs/src/workdir.py | Adds get_rebuild_out_dir() path helper for rebuild artifact storage. |
| oss_crs/src/utils.py | Adds deterministic preserved-builder and snapshot tag helpers. |
| oss_crs/src/ui.py | Excludes preserved builder images from docker_compose_down cleanup. |
| oss_crs/src/templates/run-crs-compose.docker-compose.yaml.j2 | Injects builder/runner sidecars and mounts rebuild-out dir into CRS modules. |
| oss_crs/src/templates/renderer.py | Wires rebuild-out dir + incremental build into compose rendering context. |
| oss_crs/src/templates/oss_crs_handler.sh | Updates template handler script to accept patch path + dynamic build command. |
| oss_crs/src/target.py | Removes legacy target snapshot creation logic. |
| oss_crs/src/env_policy.py | Adds OSS_FUZZ_TARGET_ENV mapping + injects rebuild-out dir + BUILDER_MODULE. |
| oss_crs/src/crs.py | Preserves builder images via deterministic retagging during build-target. |
| oss_crs/src/crs_compose.py | Adds snapshot creation/validation/cleanup for incremental runs. |
| oss_crs/src/constants.py | Adds PRESERVED_BUILDER_REPO constant for preserved builder images. |
| oss_crs/src/config/crs.py | Removes snapshot/run_snapshot fields; makes dockerfile mandatory for run modules. |
| oss_crs/src/cli/crs_compose.py | Adds --incremental-build to build-target/run CLI args and passes through. |
| libCRS/README.md | Updates command list for apply-patch-test. |
| libCRS/libCRS/local.py | Implements new builder/runner sidecar flows, rebuild artifact download, new response fields. |
| libCRS/libCRS/cli/main.py | Updates CLI commands/flags for rebuild IDs and apply-patch-test. |
| libCRS/libCRS/base.py | Updates abstract interface contracts for rebuild-id-aware build outputs and new commands. |
| libCRS/install.sh | Installs libCRS both as a CLI tool and as a system package for Python API users. |
| docs/README.md | Documents --incremental-build behavior in build-target/run. |
| docs/design/libCRS.md | Updates libCRS design docs for new sidecar injection + new commands/fields. |
| docs/design/architecture.md | Updates architecture docs for new builder-sidecar functions. |
| docs/crs-development-guide.md | Updates dev guide for framework-injected sidecars + new libCRS commands/vars. |
| docs/config/crs.md | Updates CRS config docs removing snapshot/run_snapshot and noting framework sidecars. |
| CHANGELOG.md | Records breaking changes and new flags/commands/env vars. |
| builder/README.md | Removes legacy builder CRS documentation. |
| builder/oss-crs/server.Dockerfile | Removes legacy builder server Dockerfile. |
| builder/oss-crs/crs.yaml | Removes legacy builder CRS config. |
| .github/workflows/ci.yml | Replaces explicit ruff/pyright/pytest steps with uv run verify; tightens smoke test failure conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+96
to
+113
| """Submit a rebuild job. rebuild_id auto-increments if not provided.""" | ||
| patch_content = await patch.read() | ||
| job_id = _make_job_id(patch_content, builder_name) | ||
|
|
||
| if job_id in job_results: | ||
| existing = job_results[job_id] | ||
| if existing["status"] in ("queued", "running"): | ||
| return JobResponse(id=job_id, status=existing["status"]) | ||
| if existing["status"] == "done": | ||
| result = existing.get("result") | ||
| if result is not None and result.get("exit_code") == 0: | ||
| return existing | ||
|
|
||
| # Resolve rebuild_id if not provided | ||
| if rebuild_id is None: | ||
| rebuild_out_dir = Path(os.environ.get("REBUILD_OUT_DIR", "/rebuild_out")) | ||
| rebuild_id = next_rebuild_id(rebuild_out_dir, crs_name) | ||
|
|
There was a problem hiding this comment.
job_id is derived only from patch content (+ builder_name) and ignores a caller-supplied rebuild_id. If a client retries the same patch with a different rebuild_id, the server can return a cached successful result pointing at the previous rebuild’s artifacts, violating the request and potentially overwriting/misrouting outputs. Consider either including rebuild_id in the cache key when provided, or rejecting rebuild_id when a cached job already exists, or making rebuild_id generation fully server-side only.
Comment on lines
+61
to
+65
| job_results: dict[str, dict] = {} | ||
| _executor = ThreadPoolExecutor( | ||
| max_workers=int(os.environ.get("MAX_PARALLEL_JOBS", "4")) | ||
| ) | ||
|
|
There was a problem hiding this comment.
job_results is a shared global dict that is read/updated by both the request handlers and the ThreadPoolExecutor worker threads without synchronization. This can lead to races (e.g., status/result updates being lost or partially written) under concurrent load. Add a threading.Lock (or use a thread-safe structure) around all accesses/mutations of job_results (including in _run_job).
Comment on lines
+68
to
+76
| # Resolve harness path: /out/{crs_name}/{rebuild_id}/build/{harness_name} | ||
| # Build artifacts are placed under build/ by submit-build-output. | ||
| rebuild_out = os.path.join(out_dir, crs_name, str(rebuild_id), "build") | ||
| harness_path = os.path.join(rebuild_out, harness_name) | ||
| if not os.path.exists(harness_path): | ||
| return JSONResponse( | ||
| status_code=404, | ||
| content={"error": f"Harness binary not found: {harness_path}"}, | ||
| ) |
There was a problem hiding this comment.
crs_name and harness_name are interpolated into filesystem paths and can contain path separators like ../, allowing path traversal outside the intended /out/{crs}/{rebuild_id}/build/ tree (and reading arbitrary files from the bind-mounted host directory). Validate these inputs (e.g., reject path separators, enforce a strict filename regex, and/or use os.path.realpath checks to ensure the resolved path stays under rebuild_out).
Comment on lines
+1
to
+27
| #!/bin/sh | ||
| # Usage: verify-patch-inc.sh <fuzz-proj-path> <harness> <diff> <pov-dir> [compose-file] | ||
| set -eu | ||
|
|
||
| PROJ=${1:?Usage: $0 <fuzz-proj-path> <harness> <diff> <pov-dir> [compose-file]} | ||
| HARNESS=${2:?} | ||
| DIFF=${3:?} | ||
| POV_DIR=${4:?} | ||
|
|
||
| COMPOSE=${5:-example/crs-verify-patch/compose.yaml} | ||
| BUILD_ID="vp-$(basename "$PROJ")" | ||
|
|
||
| uv run oss-crs prepare --compose-file "$COMPOSE" | ||
| uv run oss-crs build-target --compose-file "$COMPOSE" \ | ||
| --incremental-build \ | ||
| --build-id "$BUILD_ID" \ | ||
| --fuzz-proj-path "$PROJ" | ||
| uv run oss-crs run --compose-file "$COMPOSE" \ | ||
| --fuzz-proj-path "$PROJ" \ | ||
| --target-harness "$HARNESS" \ | ||
| --diff "$DIFF" \ | ||
| --incremental-build \ | ||
| --build-id "$BUILD_ID" \ | ||
| --run-id "$BUILD_ID" \ | ||
| --pov-dir "$POV_DIR" | ||
| RUN_RC=$? | ||
|
|
There was a problem hiding this comment.
Because the script runs with set -e, a non-zero exit from uv run oss-crs run ... will terminate the script immediately and RUN_RC=$? will never execute. If the intent is to always print timing logs and then exit with the run status, wrap the oss-crs run invocation with set +e / set -e (or use RUN_RC=0; ... || RUN_RC=$?).
Comment on lines
+1
to
+25
| #!/bin/sh | ||
| # Usage: verify-patch-no-inc.sh <fuzz-proj-path> <harness> <diff> <pov-dir> [compose-file] | ||
| set -eu | ||
|
|
||
| PROJ=${1:?Usage: $0 <fuzz-proj-path> <harness> <diff> <pov-dir> [compose-file]} | ||
| HARNESS=${2:?} | ||
| DIFF=${3:?} | ||
| POV_DIR=${4:?} | ||
|
|
||
| COMPOSE=${5:-example/crs-verify-patch/compose.yaml} | ||
| BUILD_ID="vp-$(basename "$PROJ")" | ||
|
|
||
| uv run oss-crs prepare --compose-file "$COMPOSE" | ||
| uv run oss-crs build-target --compose-file "$COMPOSE" \ | ||
| --build-id "$BUILD_ID" \ | ||
| --fuzz-proj-path "$PROJ" | ||
| uv run oss-crs run --compose-file "$COMPOSE" \ | ||
| --fuzz-proj-path "$PROJ" \ | ||
| --target-harness "$HARNESS" \ | ||
| --diff "$DIFF" \ | ||
| --build-id "$BUILD_ID" \ | ||
| --run-id "$BUILD_ID" \ | ||
| --pov-dir "$POV_DIR" | ||
| RUN_RC=$? | ||
|
|
There was a problem hiding this comment.
Because the script runs with set -e, a non-zero exit from uv run oss-crs run ... will terminate the script immediately and RUN_RC=$? will never execute. If the intent is to always print timing logs and then exit with the run status, wrap the oss-crs run invocation with set +e / set -e (or use RUN_RC=0; ... || RUN_RC=$?).
Comment on lines
351
to
356
| run_pov_parser.add_argument( | ||
| "--builder", | ||
| type=str, | ||
| required=True, | ||
| help="Builder sidecar module name (resolved to URL via get-service-domain)", | ||
| default=None, | ||
| help="Runner sidecar module name (defaults to BUILDER_MODULE env var)", | ||
| ) |
There was a problem hiding this comment.
The run-pov CLI flag is still named --builder and its help text says it defaults to BUILDER_MODULE, but the implementation treats this value as the runner sidecar service name (and LocalCRSUtils defaults to RUNNER_MODULE / runner-sidecar). This is confusing and can lead to misconfiguration; consider renaming the flag/help to --runner (or at least change the help text and wiring to reference RUNNER_MODULE).
Collaborator
Author
There was a problem hiding this comment.
Need to figure out what RUNNER_MODULE is about. builder as CLI arg is fine.
Comment on lines
+70
to
+86
| base_image = _TEST_IMAGE | ||
| build_id = "integration-test-001" | ||
| builder_name = "integ-builder" | ||
| crs_name = "integ-crs" | ||
| rebuild_out_dir = tmp_path / "rebuild_out" | ||
| rebuild_out_dir.mkdir() | ||
|
|
||
| # Create a minimal patch (empty is fine for lifecycle test) | ||
| result = run_ephemeral_build( | ||
| base_image=base_image, | ||
| rebuild_id=build_id, | ||
| builder_name=builder_name, | ||
| crs_name=crs_name, | ||
| patch_bytes=b"", | ||
| rebuild_out_dir=rebuild_out_dir, | ||
| timeout=60, | ||
| ) |
There was a problem hiding this comment.
run_ephemeral_build is annotated/implemented with rebuild_id: int, but this test passes string values (e.g. build_id = "integration-test-001" and then rebuild_id=build_id). This makes the test inconsistent with the service/API contract (and can hide bugs related to numeric ID allocation). Use integer rebuild IDs in these tests (and keep build_id as a separate string if needed).
Comment on lines
+182
to
+195
| def test_container_cleanup_on_failure(tmp_path): | ||
| """TST-02: Container is removed even when build fails.""" | ||
| from docker_ops import run_ephemeral_build | ||
|
|
||
| # Use base image to force failure at command execution (no oss_crs_handler.sh) | ||
| result = run_ephemeral_build( | ||
| base_image=_TEST_IMAGE, | ||
| rebuild_id="cleanup-test", | ||
| builder_name="integ-builder", | ||
| crs_name="integ-crs", | ||
| patch_bytes=b"", | ||
| rebuild_out_dir=tmp_path, | ||
| timeout=10, | ||
| ) |
There was a problem hiding this comment.
run_ephemeral_build is annotated/implemented with rebuild_id: int, but this test passes a string (rebuild_id="cleanup-test"). If rebuild_id is intended to be numeric for directory scanning and API consistency, keep tests aligned and pass an int here as well (or broaden the function signature/validation to accept strings explicitly).
Collaborator
Author
There was a problem hiding this comment.
rebuild_id should be string, not int
Comment on lines
+202
to
+222
| try: | ||
| container = client.containers.create( | ||
| base_image, | ||
| command=[ | ||
| "bash", | ||
| "/usr/local/bin/oss_crs_handler.sh", | ||
| "/OSS_CRS_BUILD_OUT_DIR/patch.diff", | ||
| "/out", | ||
| ], | ||
| volumes=volumes, | ||
| environment={ | ||
| "OSS_CRS_BUILD_ID": str(rebuild_id), | ||
| "OSS_CRS_REBUILD_ID": str(rebuild_id), | ||
| "OSS_CRS_BUILD_CMD": build_cmd, | ||
| "OSS_CRS_BUILD_OUT_DIR": "/OSS_CRS_BUILD_OUT_DIR", | ||
| "OSS_CRS_PROJ_PATH": "/OSS_CRS_PROJ_PATH", | ||
| "OSS_CRS_NAME": crs_name, | ||
| "OSS_CRS_RUN_ENV_TYPE": "local", | ||
| "REPLAY_ENABLED": "", | ||
| **_oss_fuzz_env(), | ||
| }, |
There was a problem hiding this comment.
The ephemeral container env sets OSS_CRS_BUILD_ID to rebuild_id. Elsewhere OSS_CRS_BUILD_ID refers to the operator-supplied build-target build ID (used for snapshot tags), while OSS_CRS_REBUILD_ID refers to the per-patch rebuild. To avoid confusing tooling/scripts inside the builder image, keep OSS_CRS_BUILD_ID consistent with the build-target build ID (or omit it here) and rely on OSS_CRS_REBUILD_ID for the per-rebuild identifier.
Comment on lines
+243
to
+248
| # Sidecar services are always injected as infrastructure (per D-04: single parent mount) | ||
| rebuild_out_dir = str( | ||
| crs_compose.work_dir.get_rebuild_out_dir( | ||
| crs_compose.crs_list[0].name, target, run_id, sanitizer, create=True | ||
| ) | ||
| ) |
There was a problem hiding this comment.
rebuild_out_dir is computed using crs_compose.crs_list[0].name, but the sidecar volume is shared across all CRSs in the run. This makes the on-host storage location depend on list ordering and nests shared infra state under a single CRS’s run directory. Consider adding a WorkDir helper for a run-scoped rebuild-out root (not CRS-scoped) and use that here to avoid surprising paths/collisions when multiple CRSs are present.
Collaborator
Author
There was a problem hiding this comment.
Need to further investigate what this returned rebuild out dir is used for. Ideally all CRSs should have their dirs used in this context.
acorn421
force-pushed
the
feat/125-sidecar
branch
from
f18de48 to
0810201
Compare
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
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.
Summary
Refactors the builder sidecar architecture per #125. Instead of CRS-declared long-running builder containers with snapshot configs (
snapshot: true/run_snapshot: true), the framework now:crs.yaml--incremental-buildflag (operator-controlled) tobuild-targetandrunfor Docker snapshot-based fast rebuildsrun-testtoapply-patch-test— patches are now applied in a fresh container rather than reusing a stateful buildBreaking changes
snapshotandrun_snapshotfields removed fromcrs.yamlOSS_CRS_SNAPSHOT_IMAGEenv var removedlibCRS run-testreplaced bylibCRS apply-patch-test--build-idin sidecar commands replaced by--rebuild-idbuild_exit_code→retcode,build_id→rebuild_idTest plan
test_builder_sidecar_full— E2E: prepare → build-target → run with patch, POV, and test.sh through the full sidecar pipeline. Tests multiple builder configs (default-build + coverage-build) to verify custom/multi-builder support, coverage instrumentation verification, and rebuild artifact download.test_builder_sidecar_lite— E2E: prepare → build-target → run with patch verification (build, rebuild, POV, test)test_sidecar_lifecycle— Ephemeral container lifecycle: start, build, snapshot creation/reuse, cleanup on failuretest_env_policy— Framework injectsBUILDER_MODULEand CRS cannot override itapply-patch-build/run-pov/apply-patch-test)