Skip to content

refactor(docker): one module spells the docker CLI - #453

Merged
napetrov merged 3 commits into
intel:mainfrom
napetrov:arch-c3-container
Nov 7, 2026
Merged

napetrov merged 3 commits into
intel:mainfrom
napetrov:arch-c3-container

Conversation

@napetrov

@napetrov napetrov commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

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.

dokimi/docker_cmd.py            (foundation layer, starts nothing)
  BINARY, timeouts, PIDS_LIMIT, PROXY_BUILD_ARGS, INFRA_EXIT
  TASK_CONTAINER_HARDENING, Docker alias
  argv(), mount_args(), env_args(), build_args(),
  inspect_image_args(), kill_args(), failed_to_start()
    ^
    | used by
repos/container.py        DockerRunner (argv, run, build, image_exists)
harnesses/container_agent.py  exec argv, hardening tuple
harnesses/docker_solver.py    _docker step
harnesses/fidelity.py         run_docker
harnesses/benchflow.py        four probes
metrics/example_runner.py     docker backend
commands/repo_run.py          --build-timeout default = docker_cmd.BUILD_TIMEOUT_S

Unchanged: every CLI flag, default value (--build-timeout 3 h), argv content and order, network policy (harnesses/network_policy.py still decides the network and passes it in as a string).

Not moved: _build_image and _start_container in docker_solver.py still build their argv inline; ContainerPolicy and TASK_CONTAINER_HARDENING are still two hardening spellings.

Evidence

  • Before: pytest tests on intel/main 8a6207b: 16676 passed, 12 skipped.
    After: pytest tests on 3cb13f4: 16691 passed, 12 skipped (+15 in tests/test_docker_cmd.py, fake runner, no docker).
  • Argv characterization tests (tests/test_container_argv_characterization.py) unchanged and passing.
  • New AST guard test_no_docker_argv_is_spelled_outside_the_module fails if a list or tuple starting with the literal "docker" appears outside docker_cmd.py.
  • mypy clean on the package; ruff clean.

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 --docker override still reaches only DockerRunner (pre-existing, unchanged). The re-exported names in repos/container.py are copies for existing importers; they are not what the code reads, so patching them has no effect (no test does).

🤖 Generated with Claude Code

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>
@napetrov
napetrov requested a review from a team as a code owner November 7, 2026 07:43
Copilot AI balanced review requested due to automatic review settings November 7, 2026 07:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 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-commenter

codecov-commenter commented Nov 7, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.10%. Comparing base (8a6207b) to head (9ba725c).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
dokimi/dokimi/repos/container.py 93.33% 0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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     
Files with missing lines Coverage Δ
dokimi/dokimi/commands/repo_run.py 95.73% <100.00%> (+0.52%) ⬆️
dokimi/dokimi/docker_cmd.py 100.00% <100.00%> (ø)
dokimi/dokimi/harnesses/benchflow.py 95.43% <100.00%> (ø)
dokimi/dokimi/harnesses/container_agent.py 97.87% <100.00%> (-0.02%) ⬇️
dokimi/dokimi/harnesses/docker_solver.py 97.74% <100.00%> (ø)
dokimi/dokimi/harnesses/fidelity.py 95.75% <100.00%> (ø)
dokimi/dokimi/metrics/example_runner.py 95.91% <100.00%> (ø)
dokimi/dokimi/repos/container.py 92.10% <93.33%> (+7.18%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-local with 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, two test_audit_test_quality fixtures) 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.py against the docker tests, the argv characterization tests and test_repo_runner.py: 15 mutants. 12 are caught by those files. Two more (SETGID dropped, STEP_TIMEOUT_S changed) are pinned in test_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; ValueError is re-raised as ContainerError.
  • 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.py pulls in only the small docker_cmd module. The set of dokimi.repos modules loaded at startup is the same as at the base.
  • Repo rules: changelog fragment present, no generated artifacts, commit has Signed-off-by.

Comment thread dokimi/dokimi/docker_cmd.py Resolved
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

@antonwolfy antonwolfy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 asserts build_args emits --build-arg for each PROXY_BUILD_ARGS name in order.
  • 9ba725c: dokimi/docs/architecture.md:76: added docker_cmd to the foundation row; dokimi/docs/coding-harnesses.md:347 now names docker_cmd.py as where TASK_CONTAINER_HARDENING is defined.

make ci-local at 9ba725c: 37 passed, 0 failed, 12 skipped.

@napetrov
napetrov merged commit 15d840d into intel:main Nov 7, 2026
31 checks passed
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.

4 participants