Repository navigation
refactor(docker): one module spells the docker CLI - #453
Conversation
Every docker argv in the package now comes from dokimi.docker_cmd, and every docker process is started through dokimi.process. The module owns the binary name, the step and build timeouts, the mount and -e spellings, the task-container hardening, the build's proxy build-args and the infra exit code. Callers keep their policy and their flag order. Routed: repos/container.py (DockerRunner argv, build, inspect, kill), harnesses/docker_solver.py (_docker), harnesses/container_agent.py (exec argv, constants), harnesses/fidelity.py (run_docker), harnesses/benchflow.py (four probes), metrics/example_runner.py (docker backend). The --build-timeout default is read from the module. A guard test fails on a second literal "docker" argv in the package. Tests use a FakeRunner; none needs a docker daemon. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the refactor centralizing shared Docker command construction.
No open issues 🎉
What changed in this PR
0 new findings; nothing to fix.
This refactor centralizes shared Docker CLI construction in docker_cmd.py, while keeping execution in dokimi.process and policy decisions with callers.
Changes:
- Consolidates Docker constants, argument helpers, and task-container hardening.
- Updates Docker callers and the build-timeout default without changing flags or argument order.
- Adds helper tests and an AST guard against inline Docker command literals.
| File | Description |
|---|---|
dokimi/tests/test_import_architecture.py |
Registers docker_cmd in the foundation layer. |
dokimi/tests/test_docker_cmd.py |
Tests helpers, runner behavior, and centralized command construction. |
dokimi/dokimi/repos/container.py |
Uses shared helpers and preserves constant re-exports. |
dokimi/dokimi/metrics/example_runner.py |
Uses the shared command builder for Docker execution. |
dokimi/dokimi/harnesses/fidelity.py |
Uses the shared command builder for fidelity runs. |
dokimi/dokimi/harnesses/docker_solver.py |
Routes Docker steps through the shared command builder. |
dokimi/dokimi/harnesses/container_agent.py |
Imports shared hardening, timeout, and callable definitions; updates exec arguments. |
dokimi/dokimi/harnesses/benchflow.py |
Updates Docker probes and cleanup commands. |
dokimi/dokimi/docker_cmd.py |
Defines shared Docker constants and argument helpers without starting processes. |
dokimi/dokimi/commands/repo_run.py |
Sources the build-timeout default from the shared constant. |
dokimi/changelog.d/20261107_120000_napetrov_docker-cmd-seam.md |
Documents the behavior-preserving refactor. |
Review effort: Balanced
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
==========================================
+ Coverage 94.99% 95.10% +0.10%
==========================================
Files 366 373 +7
Lines 48636 49278 +642
Branches 8373 8467 +94
==========================================
+ Hits 46204 46867 +663
+ Misses 1539 1529 -10
+ Partials 893 882 -11
🚀 New features to boost your workflow:
|
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. The refactor keeps argv content, flag order, timeouts and error messages at every call site, and the new tests pin the shared helpers. Two nits, one inline and one in the docs.
What I ran, on a Linux review host at 3cb13f4:
make ci-localwith docker: 33 passed, 4 failed, 12 skipped. The suite ran 16626 passed, 2 failed, 2 errors; the four are pytest-timeout and a 1s timing assertion (test_the_interval_excludes_zero_exactly_when_p_is_below_005,test_unbalanced_quote_in_hook_completes_quickly, twotest_audit_test_qualityfixtures) under full-suite load. They pass when run alone (701 passed across their three files) and the PR does not touch them. Changed-line coverage passed. The other two FAILs are the gitleaks install and scan (the tool is not available on the runner) and the windows-smoke step (a ResourceWarning in pytest's temp-dir cleanup after all its tests passed); neither involves this diff.- Mutations of
docker_cmd.pyagainst the docker tests, the argv characterization tests andtest_repo_runner.py: 15 mutants. 12 are caught by those files. Two more (SETGID dropped,STEP_TIMEOUT_Schanged) are pinned intest_docker_solver_harness.py. One survives (the nit below).
Nit on docs. docs/architecture.md:76 lists the foundation modules without docker_cmd, though tests/test_import_architecture.py now puts it in that layer. docs/coding-harnesses.md:347 says TASK_CONTAINER_HARDENING lives in harnesses/container_agent.py; it is defined in docker_cmd.py and only re-exported there. Suggestion: add docker_cmd to the foundation row and name docker_cmd.py in that sentence.
Checked, no issues
- Argv parity:
DockerRunner.argv/run/build/image_exists,exec,_docker,run_docker, the benchflow probes and the example runner build the same argv as the base. - Mount error: the comma-in-mount message is identical;
ValueErroris re-raised asContainerError. - Re-exports: the one test reading moved names (
test_repo_runner.py:541) still passes. No test patches the moved constants. - Startup imports:
commands/repo_run.pypulls in only the smalldocker_cmdmodule. The set ofdokimi.reposmodules loaded at startup is the same as at the base. - Repo rules: changelog fragment present, no generated artifacts, commit has
Signed-off-by.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
antonwolfy
left a comment
There was a problem hiding this comment.
LGTM. I pushed a commit per nit, up to 9ba725c, which fix the two nits from my review:
0dcb8d8:dokimi/tests/test_docker_cmd.py:49: added a test that sets all six proxy variables and assertsbuild_argsemits--build-argfor eachPROXY_BUILD_ARGSname in order.9ba725c:dokimi/docs/architecture.md:76: addeddocker_cmdto the foundation row;dokimi/docs/coding-harnesses.md:347now namesdocker_cmd.pyas whereTASK_CONTAINER_HARDENINGis defined.
make ci-local at 9ba725c: 37 passed, 0 failed, 12 skipped.
Summary
One module,
dokimi/docker_cmd.py, now spells the docker CLI. Call sites keep their own flags and order; the seam owns the shared pieces.Unchanged: every CLI flag, default value (
--build-timeout3 h), argv content and order, network policy (harnesses/network_policy.pystill decides the network and passes it in as a string).Not moved:
_build_imageand_start_containerindocker_solver.pystill build their argv inline;ContainerPolicyandTASK_CONTAINER_HARDENINGare still two hardening spellings.Evidence
pytest testson intel/main 8a6207b: 16676 passed, 12 skipped.After:
pytest testson 3cb13f4: 16691 passed, 12 skipped (+15 intests/test_docker_cmd.py, fake runner, no docker).tests/test_container_argv_characterization.py) unchanged and passing.test_no_docker_argv_is_spelled_outside_the_modulefails if a list or tuple starting with the literal"docker"appears outsidedocker_cmd.py.Merge Danger
Door: two-way (a pure refactor; revert restores the inline spellings).
Blast Radius: docker-backed runs only (repo container runs, task containers, fidelity, benchflow probes, example runner). No CLI flag, default, persisted format or exit code changes.
Ramification to check: the
--dockeroverride still reaches onlyDockerRunner(pre-existing, unchanged). The re-exported names inrepos/container.pyare copies for existing importers; they are not what the code reads, so patching them has no effect (no test does).🤖 Generated with Claude Code