refactor(sandbox): deduplicate docker not found message and name timeouts (#41)

[ulises-jeremias]

What this changes

Fixes #41 — two small copy-paste debts in the container layer:

• One constant for the Docker message: DOCKER_NOT_FOUND_MSG = "docker CLI not found — install Docker and ensure it is on PATH" defined in sandbox.py, imported in render.py and cli.py, used at all 4 sites (sandbox._run, render.check_render_image, render.run_render, cli._preflight). Previously the CLI wording was on PATH — Docker Desktop and would drift.
• Named timeouts: _START_TIMEOUT_S=120, _CP_TIMEOUT_S=300, _DESTROY_TIMEOUT_S=60, _SOCKET_GID_TIMEOUT_S=60 in sandbox.py; _CHECK_TIMEOUT_S=120, _GIF_TIMEOUT_S=300 in render.py. Making them config knobs is out of scope — named constants are enough.

Pure refactor, no behavior change; python -m pytest tests/ -q passes untouched.

Closes #41.

The grounding invariant

• Does not let an unverified command reach verified artifacts — container-layer plumbing only.
• Sandbox hardening flags unchanged (values identical, just named).

Tests

• python -m pytest tests/ -q 716 passed (1 pre-existing unrelated)
• ruff check src/readme2demo/sandbox.py src/readme2demo/render.py src/readme2demo/cli.py clean

Prompt changes

• N/A

Housekeeping

• One concern per PR.

[alphacrack]

Nice, tightly-scoped refactor — this is the right shape for #41. Specifically well done:

• The constant lives in sandbox.py and the two render.py raise sites (render.py:86, render.py:161) now share it, so the container layer and the CLI can no longer drift apart.
• Picking the sandbox/render wording over the CLI's Docker Desktop and retry is the correct direction — it's platform-neutral and matches what a Linux operator actually needs to do.
• Hardening flags are genuinely untouched: the start() hunk changes only the timeout= argument; --cap-drop ALL / --security-opt no-new-privileges / --memory / --pids-limit / --network (sandbox.py:124-129) are byte-identical. No grounding surface is touched at all — this is host-side subprocess plumbing, nowhere near distill/tutorial/normalize.
• _preflight routes the message through escape() at cli.py:653 and the string carries no brackets, so failure-class 15 isn't in play.

One blocking item — the PR misses the site the issue lists first.

sandbox._run still hardcodes the literal, 90 lines below the constant it was supposed to use:

# src/readme2demo/sandbox.py:113-115
        except FileNotFoundError as e:  # docker not installed
            raise SandboxError(
                "docker CLI not found — install Docker and ensure it is on PATH"
            ) from e

The PR body and the commit message both say the constant is "used at all 4 sites (sandbox._run, render.check_render_image, render.run_render, cli._preflight)" — it's used at 3, and sandbox.py now carries the string twice, which is precisely the drift #41 exists to prevent. Fix is one line: raise SandboxError(DOCKER_NOT_FOUND_MSG) from e.

Nothing catches this because tests/test_sandbox.py:187 matches on the substring "docker CLI not found", which passes against either copy. If you want the dedup to stay deduped, tighten that assertion to the constant itself — pytest.raises(SandboxError, match=re.escape(DOCKER_NOT_FOUND_MSG)) — so a future edit to one copy fails loudly rather than silently forking again.

Nits (take or leave):

1. render._mp4_duration_s still has a bare timeout=60 (render.py:209). sandbox/render: deduplicate the 'docker CLI not found' message and name the magic subprocess timeouts #41 scoped the magic timeouts to sandbox.py, so this is defensible as out of scope — but since you did name _CHECK_TIMEOUT_S and _GIF_TIMEOUT_S in render.py, leaving exactly one bare number in the same file is the inconsistent outcome. _FFPROBE_TIMEOUT_S = 60 finishes the thought.
2. cli.py:31 puts from readme2demo.sandbox import ... ahead of from readme2demo.orchestrator import ..., breaking the alphabetical order of that block. ruff won't flag it (select = ["E9", "F"] — no isort rules), so this is convention only.
3. problems.append(DOCKER_NOT_FOUND_MSG) now fits on one line; the three-line wrap at cli.py:644-646 is a leftover from the longer literal.
4. sandbox/render: deduplicate the 'docker CLI not found' message and name the magic subprocess timeouts #41 also lists a commit 300s timeout — there is no commit method in sandbox.py (methods are _run, start, exec, copy_in, copy_out, destroy). Correct to skip it; worth a line in the PR body so the next reader doesn't go hunting.

On "no behavior change": the CLI preflight string does change for users (docker CLI not found on PATH — install Docker Desktop and retry. → the shared wording). You call it out in the prose but the summary line says pure refactor — worth reconciling. It's safe: tests/test_cli.py:415 is a negative substring assertion that still holds, and nothing in the suite pins the old text or asserts raw Rich output for it (so no 80-col CI wrap trap).

Merge order — this is why I'd sequence it rather than land it now. Base is current origin/main (merge-base == 515ab64), so no conflict against main. But git merge-tree shows a content conflict in render.py against #266, which rewrites the exact subprocess.run(cmd, capture_output=True, timeout=300) line in _generate_gif_preview that this PR converts to _GIF_TIMEOUT_S. #273 merges clean against this, but it's editing the same _preflight docker block and adds a fifth message variant (Docker daemon is not running — start Docker Desktop (or dockerd) and retry.) — once this lands, #273 should route through DOCKER_NOT_FOUND_MSG for its not-installed branch rather than keeping the old literal, or #41 reopens itself immediately.

Next step: fold DOCKER_NOT_FOUND_MSG into sandbox._run, tighten tests/test_sandbox.py:187 to the constant, and update the body's "all 4 sites" claim. Then merge after #266 (trivial rebase — one line) and give #273's author a heads-up to use the constant. Zero release risk either way; this just isn't on the 0.8.0 critical path.

[ulises-jeremias]

Addressed review (c6af240) — corrected formatting:

• sandbox.py:114: use DOCKER_NOT_FOUND_MSG constant (4th site, _run) instead of literal
• tests/test_sandbox.py: match=re.escape(DOCKER_NOT_FOUND_MSG) with import re
• render.py: extract _FFPROBE_TIMEOUT_S=60 for ffprobe subprocess (completing 4 timeout constants).

37/37 sandbox tests pass, mypy clean. Thanks!

[alphacrack]

Verified independently — merging

Re-verified on a worktree rebased onto current main (not just trusting the green check), because a green suite is not evidence a change works — that is exactly how three promo bugs shipped past 720 passing tests this week.

rebase onto main	clean
suite	750 passed, 1 warning in 3.68s (baseline on origin/main in a parallel worktree: also 750 passed — the PR adds no new test, it tightens one e
ruff	All checks passed! (.venv/bin/ruff check src/ tests/, ruff 0.16.2). Note repo
mypy	Success: no issues found in 26 source files
earlier review concern	resolved

On the earlier review finding

RESOLVED by follow-up commit 871bdd4 "fix(sandbox,render): use DOCKER_NOT_FOUND_MSG constant, timeout constants (#41)", which is on the branch head. src/readme2demo/sandbox.py:113 now reads raise SandboxError(DOCKER_NOT_FOUND_MSG) from e — the ~90-lines-below hardcoded literal is gone. Verified by grep: grep -rn "docker CLI not found" src/ returns exactly ONE hit, the constant definition itself at src/readme2demo/sandbox.py:22. All four consumers reference the symbol (sandbox.py:113, render.py:87, render.py:162, cli.py:645). The only tests/ hit is a pre-existing negative assertion (tests/test_cli.py:415 assert "docker CLI not found" not in out), which is unaffected. Timeout literals likewise fully converted in the two files in scope: grep -rn "timeout=[0-9]" src/ returns zero hits in sandbox.py and render.py; the 4 survivors are in promo.py:1066, ingest.py:220, agent.py:172, engines/openhands.py:306 — out of this PR's stated scope. Same commit also caught a site the PR body did not claim: render._mp4_duration_s's bare timeout=60 became _FFPROBE_TIMEOUT_S.

What I actually ran

Ran two scripts against the rebased worktree, plus a 6-case mutation sweep. (1) ALL FOUR MESSAGE SITES, docker absent (subprocess.run patched to raise FileNotFoundError): sandbox._run via Sandbox.start() -> SandboxError('docker CLI not found — install Docker and ensure it is on PATH'); render.check_render_image -> RenderError(same); render.run_render -> RenderError(same); cli._preflight(Config(dry_run=False, llm_backend="claude-cli")) with shutil.which("docker")->None printed "✗ docker CLI not found — install Docker and ensure it is on PATH". Script reported: "distinct raise-site messages: 1" and "cli printed text contains constant: True". So the PR's central claim is true at runtime, and the old CLI wording ("on PATH — install Docker Desktop and retry.") is gone. (2) ALL SEVEN TIMEOUT CONSTANTS, subprocess.run spied to record the timeout kwarg — sandbox: 60 <- ['docker','run','--rm'] (socket gid), 120 <- ['docker','run','-d'] (start), 300 <- docker cp in, 300 <- docker cp out, 60 <- ['docker','rm','-f'] (destroy); render: 120 <- check probe, 300 <- gif preview, 60 <- ['ffprobe','-v','error']. Every value is byte-identical to the pre-PR literal in the diff, so this is a genuine no-op rename — no hardening/timeout weakening. (3) MUTATION SWEEP (each mutation applied, full suite run, then git checkout -- src/ tests/): M2 reverting sandbox.py:113 to a drifted literal -> FAILED tests/test_sandbox.py::TestStartFailure::test_docker_cli_missing_raises_actionable_error, 1 failed / 749 passed — the core line IS pinned, and pinned specifically because this PR changed match="docker CLI not found" (a prefix that a drifted literal would still satisfy) to match=re.escape(DOCKER_NOT_FOUND_MSG). M1 reverting the same line to a byte-identical hardcoded literal -> 750 passed (expected; undetectable and harmless). M3 reverting BOTH render.py sites to a drifted literal -> 750 passed, unpinned. M4 reverting cli.py:645 to its original pre-PR wording -> 750 passed, unpinned. M5 _CP_TIMEOUT_S 300->1 -> 750 passed; _FFPROBE_TIMEOUT_S 60->1 -> 750 passed, unpinned. Also checked no import cycle (sandbox.py imports stdlib only: shlex/subprocess/uuid/dataclasses/pathlib/typing, so render.py's new from .sandbox import ... is safe), no __all__ to update, and no stale docs referencing the replaced CLI wording (only CHANGELOG.md:219 history and scripts/self-demo.sh:36, both unrelated).

---

Verdict: MERGE-WITH-NIT. Pure refactor that genuinely works — all 4 sites verified emitting the identical constant and all 7 timeouts verified unchanged at the subprocess boundary — with the prior review concern fixed AND pinned by a tightened assertion; the nit is that the anti-drift property is pinned at only 1 of 4 sites (render.py x2 and cli.py can each re-drift with a fully green 750).

[ulises-jeremias]

Pinned the anti-drift nits:

• check_render_image and run_render FileNotFoundError paths assert re.escape(DOCKER_NOT_FOUND_MSG).
• Preflight docker-missing path asserts the same shared constant.

Ready for re-review.