ci(sandbox): run real Docker containment tests - #328
Conversation
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
Two CI blockers on exact head 0db98e0:
- When Docker is unavailable,
tests/test_sandbox_runtime_docker.pyskips all four tests and the workflow still succeeds. Add an explicit failing Docker preflight so an outage cannot produce a false green. - The workflow path filters omit direct sandbox dependencies such as
assert_ai/core/security.py. Preferassert_ai/**or include the complete dependency surface.
Until these are fixed, green CI does not reliably prove real Docker containment.
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
The two previous blockers are resolved: Docker unavailability now fails explicitly, xfail/skip cannot mask the proof, the receipt confirms four executed tests, and removing path filters covers all relevant changes. One required-check reliability blocker remains: the concurrency group uses only github.head_ref, so PRs from different forks with the same branch name can cancel each other’s required check. Please key PR runs by github.event.pull_request.number, with github.ref as the non-PR fallback.
|
Fixed the concurrency collision in |
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
The PR-number concurrency key fixes the cross-fork cancellation blocker, and the earlier Docker preflight, path coverage, xfail/skip receipt, and cleanup fixes remain sound. One new false-green path was introduced on this head: tests/test_sandbox_docker_workflow.py matches the workflow selector tests/test_sandbox_*docker*.py. Because the receipt only requires a positive test count, deleting or renaming all real container tests would still run this static YAML test and let the "real Docker containment" gate pass with one test. Please keep the static workflow test outside that selector (for example, rename it) or explicitly select/mark the real Docker containment tests, so the receipt cannot be satisfied without executing one.
|
Fixed the false-green selector on exact head |
Chang Liu (changliu2)
left a comment
There was a problem hiding this comment.
Re-reviewed exact head e80bdef. The static-test false-green blocker is resolved: the workflow now selects only tests/test_sandbox_runtime_docker.py, the regression test pins that selector, and the hosted receipt contains four executed tests with no failures, errors, or skips. The earlier Docker-daemon preflight, all-change path coverage, xfail/skip protection, cleanup proof, and PR-scoped concurrency fixes remain sound. Approved.
Problem
ASSERT's real Docker sandbox tests were opt-in, and the ordinary unit workflow could collect and skip all of them while still reporting success. A path-filtered workflow also could not serve as a reliable required check.
Fix
Add an always-present GitHub Actions workflow that:
main, avoiding path-filter omissions and pending required checks;ASSERT_RUN_DOCKER_TESTS=1but Docker cannot run;tests/test_sandbox_*docker*.pymodule rather than one hard-coded file;Verification
Sandbox: real Docker containmentsuccessfully.main, including the merged host-owned egress ledger correction.Remaining repository gate
The current
mainruleset requires review but does not require status checks. After this workflow merges,Sandbox: real Docker containmentshould be added as a required status check with strict base-branch freshness. The workflow now runs on every PR, so making it required will not leave unrelated PRs waiting for a path-filtered check that never starts.A clean local image rebuild remains blocked by the local Docker builder's external PyPI TLS failure; the clean build and test path passed in GitHub Actions.