[napetrov]

Summary

docker run argv builders (before)
  repos/container.py      DockerRunner.argv      own flag spelling
  metrics/example_runner  _run_docker            own flag spelling
  harnesses/fidelity.py   run_docker            inline -e / --network host
  harnesses/docker_solver task container        TASK_CONTAINER_HARDENING constant

docker run argv builders (after)
  docker_cmd.run_spec(ContainerPolicy, layout, image, command)
    layout = named flag groups in call order ("init", "hardening", "network", ...)

 docker_solver._start_task_container
-  run_args = ["run", "-d", "--name", c, *TASK_CONTAINER_HARDENING, *net_args, ...]
+  layout = [["-d"], ["--name", c], "init", "cap_drop", "security", "pids", "cap_add", "network", ...]
+  run_spec(task_container_policy(...), layout, image, ["sleep", lifetime])

Evidence

• Before: tests/fixtures/docker/run_argv_golden.json captured from the base commit.
  After: tests/test_docker_run_argv_differential.py replays the input grid through the public entry points and compares argv byte for byte. Passes on head. One key changed on purpose: sandbox/run_args no longer carries --network (the task policy writes it from sandbox.network), so against main that key fails and every other key matches.
• scripts/refresh_docker_run_golden.py re-takes the golden; tests/test_refresh_docker_run_golden.py checks a re-take reproduces the committed file byte for byte.
• Full dokimi suite on head (e86cc3f): 18983 passed, 20 skipped.
• Edge probe over odd inputs (mem None and negative, cpus None/0.0, unicode workdir, task memory and cpus spellings, fidelity env): base and head output identical.

Merge Danger

Door: two-way (internal refactor; revert restores the old builders)

Argv is intended to be byte-identical. Removed importable names, listed for importers (checked against main, which this branch now contains): dokimi.repos.container.ContainerPolicy, dokimi.docker_cmd.PIDS_LIMIT, dokimi.docker_cmd.TASK_CONTAINER_HARDENING, dokimi.harnesses.container_agent.TASK_CONTAINER_HARDENING, dokimi.harnesses.network_policy.start_args, ContainerPolicy(memory_mb=…). The changelog fragment carries the same list. ContainerSandbox.run_args() no longer returns --network; a caller that runs its output alone gets the default bridge, so it must go through the task policy. cap_add is a new field placed before pids_limit, so positional construction shifts.

Blast Radius: docker run argv of the repo container, example runner, fidelity capture and task container.

Follow-ups

• Follow-up: drop the per-call-site layouts and let the policy order the flags, once the golden is re-taken with the refresh script (flag order is pinned by the golden today).
• Changelog: changelog.d/20261112_180000_napetrov_docker-argv-builders.md (Removed) lists the importable names that moved.
• scripts/refresh_docker_run_golden.py imports the grid from tests/, so it needs the test dependencies (pytest) installed.

🤖 Generated with Claude Code

[Copilot]

🟢 Approval recommended

No active review issues were identified for the shared Docker argument-builder refactor.

No open issues 🎉

What changed in this PR

0 new findings; nothing to fix.

This PR centralizes Docker run argument construction in docker_cmd.run_spec, using ContainerPolicy flag groups while preserving each caller’s existing argument order.

Changes:

• Migrates repository containers, example execution, fidelity capture, and task containers to the shared builder.
• Adds capability groups and task-container policy defaults; changes network selection to return a network name.
• Adds golden argument fixtures and differential tests to check byte-for-byte compatibility.

File	Description
dokimi/​tests/​test_docker_run_argv_differential.py	Adds cross-caller argument comparisons against golden fixtures.
dokimi/​tests/​test_container_policy.py	Tests capability flags, resource values, and named groups.
dokimi/​tests/​fixtures/​docker/​run_argv_golden.json	Records expected arguments and errors for the input grid.
dokimi/​dokimi/​repos/​container.py	Uses the shared builder for repository containers.
dokimi/​dokimi/​metrics/​example_runner.py	Uses the shared builder for Docker-backed examples.
dokimi/​dokimi/​harnesses/​network_policy.py	Returns the starting network name instead of flags.
dokimi/​dokimi/​harnesses/​fidelity.py	Uses shared run and environment argument builders.
dokimi/​dokimi/​harnesses/​docker_solver.py	Builds task-container arguments from policy and ordered groups.
dokimi/​dokimi/​harnesses/​container_agent.py	Uses policy-generated sandbox network flags.
dokimi/​dokimi/​docker_cmd.py	Adds the shared run_spec argument builder.
dokimi/​dokimi/​container_policy.py	Adds named flag groups, capabilities, and task-container defaults.

_{Review effort: Balanced}

[exai-bot]

OpenCodeReview: Commit f57fd1a was not reviewed. This PR has reached its review limit (6 reviews).

CI at `f57fd1a`: 15 still running, 6 passing, 2 skipped

Check	Status
Build docs (strict)	still running
CLI interface diff	still running
CodeQL (actions)	still running
CodeQL (python)	still running
External catalog smoke test (uxlfoundation/skills)	still running
Linux Python 3.11	still running
Linux Python 3.14	still running
Linux Wheels Python 3.11	still running
Linux Wheels Python 3.14	still running
Static checks (ruff, schema, zizmor, licenses, pre-commit, bandit, secrets, changelog, schema com...	still running

5 more not listed.

To review this commit: re-request review from exai-bot in the Reviewers list.

<details><summary>Last review (6e42b32): no findings</summary>

Open threads from the bot: 0

</details>

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.72%. Comparing base (f12057b) to head (f57fd1a).
⚠️ Report is 1 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #623   +/-   ##
=======================================
  Coverage   95.71%   95.72%           
=======================================
  Files         415      415           
  Lines       55346    55373   +27     
  Branches     9514     9512    -2     
=======================================
+ Hits        52977    53004   +27     
  Misses       1466     1466           
  Partials      903      903           

Files with missing lines	Coverage Δ
dokimi/dokimi/container_policy.py	100.00% <100.00%> (ø)
dokimi/dokimi/docker_cmd.py	100.00% <100.00%> (ø)
dokimi/dokimi/harnesses/container_agent.py	97.87% <100.00%> (ø)
dokimi/dokimi/harnesses/docker_solver.py	97.74% <100.00%> (ø)
dokimi/dokimi/harnesses/fidelity.py	95.72% <100.00%> (-0.04%)	⬇️
dokimi/dokimi/harnesses/network_policy.py	95.73% <100.00%> (ø)
dokimi/dokimi/metrics/example_runner.py	95.97% <100.00%> (ø)
dokimi/dokimi/repos/container.py	91.74% <100.00%> (-0.37%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vlad-perevezentsev]

Review at 8cc918b. The refactor keeps every docker run argv as it was, and the real launch confirms it. What is left is small: one stale doc name, one argv part the golden does not pin, and some cleanups, all in the line comments.

What I ran, at 8cc918b:

• make ci-local --offline: 18,397 passed, 0 failed, 72 skipped. The only FAIL is the changelog fragment check, which does not see the skip-changelog label.
• The differential test and its golden against the merge-base ead342e: it passes, so the golden is the base output.
• CI's external-catalog-smoke (docker-oracle and docker-probe on onetbb-histogram-local-aggregation): 1/1 pass each, every isolation probe closed. The task container started with the same flags in the same order as before, both on bridge and inside the egress sandbox.
• A mutation that moves the task container's -e flags after the image: all 515 tests in the docker, container, network-policy and tbench files still pass (see the line comment).

Checked, no issues

• Call sites: the repo container, the example runner, the fidelity client and the task container build the same flags in the same order as on the base; memory=f"{mem_mb}m" cannot get a None, because _run_docker returns before it when mem_mb is unset.
• bridged: network == "bridge" gives the same answer as "bridge" in net_args with and without a sandbox; test_network_policy.py fails if it is broken.
• Removed names: nothing in the tree or on current main still imports TASK_CONTAINER_HARDENING, start_args, docker_cmd.PIDS_LIMIT or memory_mb=; no call builds a ContainerPolicy by position, so the new cap_add field moves nothing.
• Changelog: no fragment, under skip-changelog; that fits, since no argv changes and the removed names are internal.

[vlad-perevezentsev]

Nit: Question: once this lands, does each call site still need its own flag order?

Docker does not care about the order of options before the image. The layouts, the group names and the split of hardening into security / cap_drop exist only to keep four old orders byte-identical. With one fixed group order in run_spec (then the caller's own flags), a later change could drop the layouts, at the cost of one golden refresh.

[napetrov]

Keeping the per-call-site layouts in this PR: the golden pins each argv's flag order, and dropping the order changes every key. With the refresh script that is cheap as a follow-up, which I will open once this lands.

[napetrov]

Round 5 is pushed at a357159 (8cc918b..a357159).

• Review XiBWSA (stale doc name): fixed in f088901.
• Question KL: the per-call-site layouts stay in this PR, since the golden pins flag order; dropping them is a follow-up, cheap now that the golden is re-taken by a script.
• PR body corrected: removed-names list against main, the one golden key that changed on purpose, changelog fragment added.

[vchamarthi]

Review at e5379c3. 2 new findings: 1 must-fix, 1 nit. In the line comments. The change moves every docker run argv the package builds onto one run_spec builder over ContainerPolicy, and the argv stays byte-identical in the tested grid. The changelog fragment's list of removed importable names omits five names this PR removes or renames.

Verified

• Argv byte identity: On e5379c3 the argv for every tested input, including repo, example and fidelity, is byte-for-byte identical to main's, and the golden refresh script's output matches the committed golden.
• Task env pinning: The -e flags of the task start in dokimi/dokimi/harnesses/docker_solver.py put TEST_DIR=/tests before the image.
• Network: run_args() no longer adds --network. The task start in dokimi/dokimi/harnesses/docker_solver.py takes the network from the task policy instead.
• Mount path comma: A , in a mount host path makes DockerRunner.argv raise ContainerError before it returns any argv, on 3b61dd0 and on e5379c3 alike.
• Static checks: On e5379c3 merged onto current main 38d07f9, ruff, the schema check, mypy, deptry, the comment-block check, schema compatibility, the mkdocs build, the CLI snapshot and the fidelity golden all pass. The typed Group literal is part of the passing mypy run.
• Merge onto main: e5379c3 merges onto current main 38d07f9 with no conflict.

[napetrov]

Re: review by vlad-perevezentsev

one stale doc name, one argv part the golden does not pin
a: f088901. b: 4ce67d1.

[napetrov]

Re: review by vchamarthi

The changelog's list of removed importable names omits five names, and two listed names were never exported
Fixed in bc35c8b: the five names are added and the two are dropped.

[vlad-perevezentsev]

LGTM

I checked 6e42b32. Every point from my first round is fixed, and the argv each call site builds is still byte-identical to main. Keeping the per-call-site layouts for a follow-up is fine with me.

Merge: safe now: it merges cleanly into current main, 10 commits behind, and its docker, container, network-policy, tbench and docs-refs tests pass on the merge (351 passed). Once it is in, #695 conflicts with it in the import block of docker_solver.py and needs a rebase.

What I ran, at 6e42b32:

• make ci-local --offline: 18,946 passed, 0 failed, 72 skipped; every check passes.
• The first round's mutation (the task container's -e flags moved after the image) fails the golden test, the new -e position test and the refresh re-take test.
• The golden against the merge-base cb6cead: only sandbox/run_args differs, the one key changed on purpose.
• CI's external-catalog-smoke (docker-oracle and docker-probe), at bc35c8b: 1/1 pass each, every isolation probe closed; the sandboxed task container starts with the same flags as before.
• On Windows (Server 2025, Python 3.13): not run. The VM venv cannot be built from the base's requirements.lock.txt (filelock==4.2.4 with litellm==1.106.4, which needs filelock<4); this PR does not touch the lock.

Checked, no issues

• The fixes: 4ce67d1, f088901, e036691, e86cc3f, fed7d97, 948153c and 61bf42e each do what their thread asked.
• Changelog: the Removed fragment names the five moved names with their replacements, and no name that main never exported.

Fixed in this branch, up to 6e42b32:

• The example's /work mount was masked whenever a file with the snippet's name was there, so a mount of another directory with a different snippet still matched the golden (the open exai-bot thread): 6e42b32 masks it only when the file holds this run's code, and a new test redirects the mount to such a directory.
  The new test fails with the old is_file() check.

[vchamarthi]

Pushed 1 commit to arch/c3b (426a610..0b5ba9a).

• The Removed bullet names docker_solver.start_args and docker_solver.TASK_CONTAINER_HARDENING with their replacements.
• The bullet says task_container_policy(network=, cpus=, memory=) returns a ContainerPolicy, not a flag list.
  Run: the full suite with main merged in, 19806 passed.
  Static checks fails on main as well: the committed docs/reference/cli-interface.json is over the 500 KB limit of the pre-commit large-file hook. fix(cli): the CLI snapshot writes one line per parameter #737 fixes it.

[xaleryb]

• Ran: the full suite, ruff, mypy, bandit, check_growth and the benchmark docs gate on 0b5ba9a (Python 3.13). All pass except Static checks.
• tests/test_changelog_docker_argv_fragment.py has no SPDX header -> the License header step fails (the open thread on line 1) -> expected: the two-line header.
• The same file reads changelog.d/20261112_180000_napetrov_docker-argv-builders.md by name -> with the fragment removed, as scriv collect does at release (scripts/check_release.py refuses while any fragment remains), both tests fail with FileNotFoundError -> expected: no test depends on a fragment that the release deletes.
• Suggested fix: drop that test file; it only checks the wording of a changelog bullet, and that settles both points.
• The other Static checks failure (check-added-large-files on docs/reference/cli-interface.json) fails on main too and is not caused by this PR.
• Earlier review points: all fixed or answered; the per-call-site layout question is deferred to a follow-up, as agreed.

[antonwolfy]

Review at 0b5ba9a. The refactor holds: every docker run argv the package builds is the same as on the base, and the new tests pin the security flags. The merge waits for the red Static checks job on GitHub, which comes from the new test_changelog_docker_argv_fragment.py.

What I ran, on a Linux review host at 0b5ba9a:

• make ci-local with docker: 33 passed, 4 failed. 19,767 tests passed; changed-line coverage, mypy, deptry, ruff, bandit, the fidelity golden and the docker-oracle/docker-probe smoke pass. The failures are not the PR's: pre-commit fails check-added-large-files on docs/reference/cli-interface.json, which this PR does not touch and which is over the limit on main too; the two gitleaks steps failed on a download; two tests in test_audit_test_quality.py hit the 60 s timeout under load and pass alone (172 passed). The PR carries the skip-changelog label.
• 60 mutants of the new code (layout groups swapped or dropped, cap list, pids limit, --init, --read-only, bridged, network): 59 are killed by the differential, policy, characterization, network-policy and container-agent tests. The survivor swaps the sandbox's --label/--tmpfs flags and the task -e flags, which only changes flag order and has no effect on behaviour.
• Base against head: the differential test and golden fail against the base only on sandbox/run_args, the one key changed on purpose.

Checked, no issues

• Call sites: the repo container, example runner, fidelity client and task container build the same flags in the same order as on the base, with and without a sandbox, and bridged=network == "bridge" matches the old "bridge" in net_args.
• Removed names: no reference to TASK_CONTAINER_HARDENING, start_args, docker_cmd.PIDS_LIMIT or ContainerPolicy(memory_mb=...) is left in the package, docs, scripts or tests.
• Commits: all 19 non-merge commits carry a Signed-off-by.

I agree with @xaleryb's points on the missing license header in test_changelog_docker_argv_fragment.py and on its dependence on the changelog fragment. New evidence: with the fragment removed from a copy of the tree, both tests fail with FileNotFoundError.

Bot points I checked that no longer apply at this head (1)

• exai-bot's point on masking the example's /work mount does not apply: the mount is masked only when the snippet file holds this run's code, and a test redirects the mount to a directory with a different snippet.

[napetrov]

Re: comment by xaleryb

Drop the test file
Dropped in 9334a4b. Large-file check passes now.

[napetrov]

Re: review by antonwolfy

The merge waits for red Static checks
Fixed in 9334a4b: test file dropped, main merged.

[napetrov]

Re: comment by vchamarthi

Static checks fails on main too
Already fixed: main's snapshot is 301 KB, merged in 8f86847.

[antonwolfy]

LGTM. The refactor holds: every docker run argv the package builds is the same as at the base, the security flags are pinned by the tests, and the commits since my last review only drop the changelog-wording test, merge main and remove two unused noqa markers. One nit inline.

What I ran, on a Linux review host at 7b0a3e9:

• make ci-local with docker: 37 passed, 1 failed, 14 skipped. 20,447 tests passed; changed-line coverage, mypy, deptry, ruff, bandit, pre-commit, the fidelity golden, the changelog-fragment check and the docker-oracle/docker-probe smoke pass. The one FAIL is test_a_fixed_finding_leaves_the_baseline_too in test_audit_test_quality.py, which hit the 60 s timeout under load; the file does not touch this PR, and run alone it passes (172 passed).
• 44 mutants of the new code (security flags, cap list, pids limit, network, bridged, read-only, layout groups, mount flags): 40 killed by the differential and policy tests. The 4 survivors are a no-op mutant, and three lines this PR did not change (failed_to_start's not timed_out, the value of INFRA_EXIT, the comma check on the container path in mount_args).
• The golden re-take script run against the base package: the output differs from the committed golden only in the --network pair under sandbox/run_args, the one key changed on purpose.

Checked, no issues

• Changelog names: every name the fragment lists was importable at the base and is not at head; a grep of docs/, scripts/ and tests/ finds no stale reference.
• Layouts: the task container's layout (init cap_drop security pids cap_add network sandbox_flags env resource) matches the base order, and bridged=network == "bridge" matches the old "bridge" in net_args.
• Commits: all 22 non-merge commits carry a Signed-off-by.
• Repo rules: new Python files carry the SPDX header, docker_solver.py and test_container_agent.py sit at their growth baseline, and the deptry entry is needed because scripts/ imports a test module.

[antonwolfy]

LGTM. I pushed a commit per nit, up to f57fd1a, which fix the nit from my review:

• f57fd1a: dokimi/changelog.d/20261112_180000_napetrov_docker-argv-builders.md:3: the bullet now says ContainerPolicy gains cap_add before pids_limit, so construct it by keyword, and memory is a string such as "512m".

make ci-local at f57fd1a: 0 failed, 38 passed, 14 skipped.