From b49121b498b148e55138663fc06c3099f6d2d6dd Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sun, 9 Aug 2026 08:22:39 +0100 Subject: [PATCH 1/3] Three-state tools probe: only the official claude layout counts as provisioned The probe asked `command -v claude` and believed the answer, which the claude-shim downloader satisfies -- so an image baking the shim (this repo's own .devcontainer/claude-code/ feature does) skipped the lend and paid the ~285MB GCS download on first use, the exact cost the lending exists to remove. The probe is now a pure `probe_script()` printing exactly one of provisioned / lendable / absent, exiting 0 in all three, read into a `ProbeResult` sum type by a total parser. It resolves the candidate's path and never executes it: on a shim, any invocation triggers the download the probe exists to detect. A lendable container is upgraded -- the host streams its real binary in and the transfer's ~/.local/bin PATH prepend makes it win, so the next launch probes provisioned. With nothing to lend, or a lend the container cannot run, it is accepted rather than falling through to the network install, which its own `command -v` guards would no-op anyway. The official layout is one module constant now, shared by _claude_source, transfer_script and probe_script -- one definition of "a real claude" on both sides of the pipe. Closes #151 --- CHANGELOG.md | 37 +++++- devlaunch/tools.py | 168 +++++++++++++++++++++---- test/unit/test_tools.py | 272 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 441 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20e16d78..bcf6fff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,9 +87,40 @@ table and should be judged on its own merits. `claude` that could never run. The probe that decides all this is captured, unlike the trips that may follow - it. It reports nothing, and its everyday answer on a cold workspace ("tools - missing") reached the terminal as a red devpod - `fatal ... Process exited with status 1` describing the probe working. + it: what it prints is the answer the caller branches on rather than progress + anybody needs to watch. + +- **A baked `claude` counts as provisioned only if it is the real one.** The + probe used to ask `command -v claude` and believe the answer, which the + `claude-shim` downloader satisfies — so an image carrying the shim (including + any built from this repo's own `.devcontainer/claude-code/` feature) skipped + the lend and paid the ~285MB GCS download on first use, the exact cost the + lending exists to remove. It now answers one of three states: `provisioned` + when `gh` is on the login `PATH` **and** `claude` resolves into + `~/.local/share/claude/versions/` — the same definition of "a real claude" + the host already applies to decide what it may lend, now written down once + and used on both sides of the pipe; `lendable` when both answer but the + claude is a shim or wrapper; `absent` otherwise. + + A `lendable` container is quietly upgraded: the host streams in its own + binary and the transfer's `~/.local/bin` `PATH` prepend is what makes it win + from then on, so the *next* launch probes `provisioned`. If the host has + nothing to lend, or the lent binaries do not run there, the container is + accepted as it stands rather than falling through to the network install — + that install decides what to do with its own `command -v` guards, which a + shim already satisfies, so the trip would install nothing. + + The probe never executes the candidate `claude`; on a shim, *any* invocation + triggers the very download the probe exists to detect. It resolves the path + instead. It also exits 0 in all three states now, which retires the red + devpod `fatal ... Process exited with status 1` that the old probe's + everyday cold answer painted on the terminal. + + Two things this deliberately does not do. It does not compare versions: a + real `claude` already in the container is left alone however old it is, since + keeping versions in sync would make this a package manager (the binary + self-updates, and a workspace rebuild re-provisions). And it does not make + the payload per-tool — an image with only `gh` still receives both. - **Two `up`s of one workspace serialize on a per-workspace lock.** Background prewarming makes concurrent `up`s of a single workspace an everyday event diff --git a/devlaunch/tools.py b/devlaunch/tools.py index c64b11b3..08248ad7 100644 --- a/devlaunch/tools.py +++ b/devlaunch/tools.py @@ -24,10 +24,20 @@ lent binaries do not run there (a different arch or libc), does the old network path run: bootstrap pixi, `pixi global install` each tool. -The round trips this costs, by path: a warm workspace pays one (the probe, -which was always paid); a cold one pays two (probe, then transfer), or three -when the transfer cannot help and the network fallback runs -- against a -`devpod up` that already ran for seconds to minutes. +What "lacks them" means is the probe's job, and it has three answers rather +than two, because a `claude` on PATH is not necessarily a `claude` worth +keeping: the shim this repo's own devcontainer feature bakes satisfies +`command -v` while still owing the GCS download on first use. So the probe +reports `provisioned` only for the official install layout, `lendable` for a +container whose claude is something else, and `absent` when a tool is really +missing -- and a lendable container is quietly upgraded to the host's real +binary the next time it goes through `up`. + +The round trips this costs, by path: a provisioned workspace pays one (the +probe, which was always paid); a lendable or cold one pays two (probe, then +transfer), or three when the transfer cannot help a genuinely empty container +and the network fallback runs -- against a `devpod up` that already ran for +seconds to minutes. Two consequences worth knowing: @@ -42,6 +52,7 @@ make an exception of. """ +import enum import json import logging import os @@ -61,6 +72,14 @@ # The claude package lives in a personal channel rather than conda-forge. BLOOOP_CHANNEL = "https://prefix.dev/blooop" +# Where the official claude installer keeps one binary per version, relative to +# a home directory. This is the definition of "a real claude" on both sides of +# the pipe -- the host reads it to decide what it may lend (`_claude_source`), +# writes into it when it lends (`transfer_script`), and the container is asked +# about it to decide whether it needs anything (`probe_script`). Stated once, +# because two copies of that definition are two probes with different opinions. +CLAUDE_VERSIONS_RELPATH = ".local/share/claude/versions" + # Which file a bash login shell will actually read. bash tries ~/.bash_profile, # ~/.bash_login and ~/.profile in that order and sources only the first that # exists, so appending to ~/.profile in an image that ships a ~/.bash_profile @@ -174,6 +193,84 @@ def provision_script(tools: Sequence[Tool] = REQUIRED_TOOLS) -> str: ) +class ProbeResult(enum.Enum): + """What one probe found in a workspace -- the whole answer, in one value. + + Three states rather than a boolean, because "there is a claude" and "there + is a claude worth keeping" are different questions, and collapsing them is + the bug this replaced: a container carrying the shim answered yes to the + first and was left owing the ~285MB download the lending exists to avoid. + A boolean plus a flag would have been the same two questions with a fourth, + meaningless combination available; three named states have no illegal one. + + The values are the tokens `probe_script` prints, so the protocol over the + ssh channel is written down once rather than once per side. + """ + + PROVISIONED = "provisioned" + LENDABLE = "lendable" + ABSENT = "absent" + + @classmethod + def parse(cls, answer: str) -> "ProbeResult": + """Read a probe's answer. Total: anything unrecognised is ABSENT. + + The last line, not the whole output: the probe runs under `bash -lc`, + so the container's login profile is sourced before the script and an + image whose profile prints a banner puts it on stdout ahead of the + token. The token is always the last thing printed. + + A garbled, empty or truncated answer has to mean something, and ABSENT + is the only state whose worst case is harmless -- provisioning is + idempotent, so reading it wrongly costs a redundant round trip, where + a wrong PROVISIONED would silently skip the work the probe exists to + schedule. + """ + lines = [line.strip() for line in answer.splitlines() if line.strip()] + if not lines: + return cls.ABSENT + try: + return cls(lines[-1]) + except ValueError: + return cls.ABSENT + + +def probe_script() -> str: + """The shell script that answers what provisioning a workspace still needs. + + Prints exactly one token -- `provisioned`, `lendable` or `absent` -- and + exits 0 in every one of them: "this container has nothing" is an answer, + not a failure, and a probe that exits non-zero paints a red devpod + `fatal ... Process exited with status 1` on the terminal of every cold + launch, describing the probe working exactly as intended. + + It asks about the fixed pair this module lends rather than an arbitrary + tool set, for the same reason `host_payload` does: what can be lent is + `gh` and `claude`, so those are what there is a three-state answer for. + """ + present = " && ".join( + f"command -v {shlex.quote(tool.command)} >/dev/null 2>&1" for tool in REQUIRED_TOOLS + ) + return "\n".join( + [ + "set -u", + f"if ! {{ {present} ; }}; then echo {ProbeResult.ABSENT.value}; exit 0; fi", + # Resolved, never run. The shim on PATH answers `command -v` just + # as the real binary does, and telling them apart by asking either + # one for its version would trigger, on the shim, the very ~285MB + # download this answer exists to avoid. Where the name resolves to + # is the whole test. `readlink` failing (or missing) leaves this + # empty, which reads as `lendable` -- the answer that does more + # work, never the one that skips it. + 'target=$(readlink -f "$(command -v claude)" 2>/dev/null || true)', + 'case "$target" in', + f' "$HOME/{CLAUDE_VERSIONS_RELPATH}/"*) echo {ProbeResult.PROVISIONED.value} ;;', + f" *) echo {ProbeResult.LENDABLE.value} ;;", + "esac", + ] + ) + + def _pixi_bootstrap() -> str: """Install pixi if the image has none, since every tool here comes from it. @@ -222,7 +319,7 @@ def _claude_source() -> Optional[Tuple[str, pathlib.Path]]: target = link.resolve(strict=True) except OSError: return None - versions_dir = (pathlib.Path.home() / ".local/share/claude/versions").resolve() + versions_dir = (pathlib.Path.home() / CLAUDE_VERSIONS_RELPATH).resolve() if target.parent != versions_dir or not os.access(target, os.X_OK): return None return target.name, target @@ -268,7 +365,7 @@ def host_payload() -> Optional[HostPayload]: return HostPayload( claude_version=version, members=( - (claude_binary, f".local/share/claude/versions/{version}"), + (claude_binary, f"{CLAUDE_VERSIONS_RELPATH}/{version}"), (gh, ".local/bin/gh"), ), ) @@ -284,7 +381,7 @@ def transfer_script(payload: HostPayload) -> str: the caller falls back to the network install. """ version = payload.claude_version - claude_rel = f".local/share/claude/versions/{version}" + claude_rel = f"{CLAUDE_VERSIONS_RELPATH}/{version}" profile_lines = "\n".join( [ _PROFILE_RESOLUTION, @@ -318,7 +415,7 @@ def transfer_script(payload: HostPayload) -> str: f'"$STAGE/{claude_rel}" --version >/dev/null', '"$STAGE/.local/bin/gh" --version >/dev/null', # Proven. Now they can be moved into place. - 'mkdir -p "$HOME/.local/bin" "$HOME/.local/share/claude/versions"', + f'mkdir -p "$HOME/.local/bin" "$HOME/{CLAUDE_VERSIONS_RELPATH}"', f'mv -f "$STAGE/{claude_rel}" "$HOME/{claude_rel}"', 'mv -f "$STAGE/.local/bin/gh" "$HOME/.local/bin/gh"', # The host's own symlink points through the host's home, so the @@ -341,22 +438,23 @@ def _write_payload_tar(payload: HostPayload, out: pathlib.Path) -> None: tar.add(source, arcname=arcname) -def _probe(workspace: str, runner, tools: Sequence[Tool]) -> bool: - """One round trip: are all `tools` already on the workspace's PATH? +def _probe(workspace: str, runner) -> ProbeResult: + """One round trip: what does this workspace still need? + + Captured, unlike the two trips that may follow -- here the output *is* the + answer the caller branches on, rather than progress a user needs to watch. - Captured, unlike the two trips that may follow. A probe has no progress to - report -- it is a yes/no question -- and "no" is the everyday answer on a - cold workspace, which devpod renders on the terminal as a red - `fatal ... Process exited with status 1`. That line describes the probe - working exactly as intended, so it is not shown. + A trip that fails is not an answer: the script exits 0 in all three states, + so a non-zero status means the ssh itself did not get through, and the + reading that costs a redundant trip is preferred to the one that skips the + work. """ - checks = " && ".join( - f"command -v {shlex.quote(tool.command)} >/dev/null 2>&1" for tool in tools - ) result = runner( - ["ssh", workspace, "--command", f"bash -lc {shlex.quote(checks)}"], capture=True + ["ssh", workspace, "--command", f"bash -lc {shlex.quote(probe_script())}"], capture=True ) - return result.returncode == 0 + if result.returncode != 0: + return ProbeResult.ABSENT + return ProbeResult.parse(result.stdout) def _transfer(workspace: str, runner, payload: HostPayload) -> bool: @@ -392,7 +490,13 @@ def ensure_tools(workspace: str, runner, tools: Sequence[Tool] = REQUIRED_TOOLS) provisioned workspace ever pays), then the host lending its own binaries (see the module docstring for why that is the fast path), then the network install for a host with nothing to lend or a container the lent binaries - cannot run in. + cannot run in. Which of them run is decided by the probe's three-state + answer, and only a genuinely `absent` container ever reaches the third. + + Version drift is deliberately not handled: a real claude already in the + container is left alone whatever its version. Keeping versions in sync + would make this a package manager, which the module docstring's "the host + first, the network second" is explicitly not. The network payload goes through `bash -lc` for the same reason workspace_ssh wraps its own: devpod runs --command under a shell that @@ -414,13 +518,33 @@ def ensure_tools(workspace: str, runner, tools: Sequence[Tool] = REQUIRED_TOOLS) return False try: - if _probe(workspace, runner, tools): + found = _probe(workspace, runner) + if found is ProbeResult.PROVISIONED: return True payload = host_payload() if payload is not None and _transfer(workspace, runner, payload): return True + if found is ProbeResult.LENDABLE: + # Both tools already answer here; only the claude was the wrong + # one, and the lend that would have replaced it either had nothing + # to send or would not run in this container. The network fallback + # cannot help either way: it decides what to install with its own + # `command -v` guards (see provision_script), which both tools + # already satisfy, so the third trip would install nothing. Stop + # with what is there. + # + # Accepted residual: a container that keeps rejecting the lend -- + # a different libc or architecture -- while carrying a shim + # re-attempts a ~5s failing transfer on every `devpod up`. Breaking + # that loop needs per-container retry state persisted somewhere, + # which is more machinery than the case is worth, and ensure_tools + # never runs on the fast-attach path: this only ever rides an `up` + # that already took seconds to minutes. + return True + + # ProbeResult.ABSENT: the cold flow, with a tool genuinely missing. script = provision_script(tools) result = runner(["ssh", workspace, "--command", f"bash -lc {shlex.quote(script)}"]) except OSError as e: diff --git a/test/unit/test_tools.py b/test/unit/test_tools.py index 7a3cb78e..c29258e3 100644 --- a/test/unit/test_tools.py +++ b/test/unit/test_tools.py @@ -1,7 +1,9 @@ """What devlaunch installs into a workspace, and what it does when that fails.""" import io +import os import shlex +import shutil import subprocess import tarfile from typing import List, Optional @@ -181,23 +183,236 @@ def fake_payload(tmp_path) -> tools.HostPayload: ) +class TestProbeScript: + """The probe runs in someone else's container and decides whether to lend. + + Checked as a string (what it may never do) and by running it for real + against scratch `$HOME`s (what it answers), because both halves matter: an + answer that is right for the wrong reason still costs a 285MB download. + """ + + @staticmethod + def _home(tmp_path): + home = tmp_path / "home" + home.mkdir() + return home + + @staticmethod + def _bare_path(tmp_path): + """A PATH carrying the coreutils the probe needs and nothing else. + + A test of "no claude here" must not find the developer's own claude, + so the real system directories are kept off PATH entirely and the one + external the probe uses is linked in by hand. + """ + sysbin = tmp_path / "sysbin" + sysbin.mkdir() + for command in ("readlink",): + found = shutil.which(command) + assert found, f"the test host needs {command}" + (sysbin / command).symlink_to(found) + return [sysbin] + + def _answer(self, tmp_path, home, path_dirs=()) -> str: + """Run the probe script for real and return the single token it printed.""" + env = { + "HOME": str(home), + "PATH": ":".join(str(d) for d in [*path_dirs, *self._bare_path(tmp_path)]), + } + # bash by absolute path: the stripped PATH the probe is handed cannot + # also be the one that finds the shell running it. + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", tools.probe_script()], + env=env, + capture_output=True, + text=True, + check=False, + ) + # Exit 0 in every state: "no tools here" is an answer, not a failure, + # and a non-zero probe paints a red devpod `fatal` on the terminal of + # every cold launch. + assert result.returncode == 0, result.stderr + return result.stdout.strip() + + @staticmethod + def _official_claude(home, version="2.0.1"): + """What the official installer leaves behind: one binary per version + under ~/.local/share/claude/versions, with ~/.local/bin/claude + pointing at the current one.""" + binary = home / ".local/share/claude/versions" / version + binary.parent.mkdir(parents=True, exist_ok=True) + binary.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + binary.chmod(0o755) + link = home / ".local/bin/claude" + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to(binary) + return link.parent + + @staticmethod + def _claude_shim(home): + """What .devcontainer/claude-code/install.sh bakes: something on PATH + answering to `claude` that fetches the real binary on first run.""" + shim = home / ".local/bin/claude" + shim.parent.mkdir(parents=True, exist_ok=True) + shim.write_text("#!/bin/sh\necho 'downloading 285MB' >&2\n", encoding="utf-8") + shim.chmod(0o755) + return shim.parent + + @staticmethod + def _gh(home): + gh = home / ".local/bin/gh" + gh.parent.mkdir(parents=True, exist_ok=True) + gh.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + gh.chmod(0o755) + return gh.parent + + def test_a_container_with_neither_tool_answers_absent(self, tmp_path): + home = self._home(tmp_path) + assert self._answer(tmp_path, home) == "absent" + + def test_the_official_claude_layout_answers_provisioned(self, tmp_path): + """The prebuilt-image jackpot: nothing to do, one round trip.""" + home = self._home(tmp_path) + self._official_claude(home) + assert self._answer(tmp_path, home, [self._gh(home)]) == "provisioned" + + def test_a_baked_shim_answers_lendable_rather_than_provisioned(self, tmp_path): + """The whole point of the three states. A `claude` that is a downloader + satisfies `command -v` while still owing the ~285MB the lending exists + to avoid, so it must not be mistaken for a provisioned workspace.""" + home = self._home(tmp_path) + self._claude_shim(home) + assert self._answer(tmp_path, home, [self._gh(home)]) == "lendable" + + def test_a_real_claude_with_no_gh_is_absent_not_lendable(self, tmp_path): + """`lendable` means "replace the claude"; a container missing gh + outright needs the cold flow, which the network fallback can finish.""" + home = self._home(tmp_path) + self._official_claude(home) + assert self._answer(tmp_path, home, [home / ".local/bin"]) == "absent" + + def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): + """Convergence, run for real end to end: the transfer script unpacks + into a scratch $HOME, and the probe is then asked about that same home. + + The two scripts have to agree or the lending never terminates -- a lend + the next probe does not recognise means every `up` for the rest of that + workspace's life re-pays the transfer. + """ + home = self._home(tmp_path) + source = tmp_path / "host" + source.mkdir() + binaries = {} + for name in ("claude-2.0.1", "gh"): + binary = source / name + binary.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + binary.chmod(0o755) + binaries[name] = binary + payload = tools.HostPayload( + claude_version="2.0.1", + members=( + (binaries["claude-2.0.1"], ".local/share/claude/versions/2.0.1"), + (binaries["gh"], ".local/bin/gh"), + ), + ) + bundle = tmp_path / "tools.tar" + with tarfile.open(bundle, mode="w") as tar: + for member_source, arcname in payload.members: + tar.add(member_source, arcname=arcname) + with open(bundle, "rb") as stream: + lend = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", tools.transfer_script(payload)], + stdin=stream, + env={**os.environ, "HOME": str(home)}, + capture_output=True, + text=True, + check=False, + ) + assert lend.returncode == 0, lend.stderr + assert self._answer(tmp_path, home, [home / ".local/bin"]) == "provisioned" + + def test_it_resolves_the_link_rather_than_trusting_the_path_entry(self): + """In the official layout `~/.local/bin/claude` is a symlink, so what + it points at is the only thing that says which install it belongs to.""" + assert "readlink -f" in tools.probe_script() + + def test_it_never_runs_the_candidate_claude(self): + """Shim-proofness is a property of the script's text, not of a run: + *any* invocation of the shim triggers the download the probe exists to + detect, so the only things the probe may do with `claude` are look its + path up and resolve it. + """ + script = tools.probe_script() + scrubbed = script.replace("command -v claude", "").replace( + f'"$HOME/{tools.CLAUDE_VERSIONS_RELPATH}/"', "" + ) + assert "claude" not in scrubbed + assert "--version" not in script + + def test_the_official_layout_is_defined_once_for_both_sides_of_the_pipe(self, tmp_path): + """The container-side "is this the official install" and the host-side + one it mirrors must not be able to drift apart.""" + assert tools.CLAUDE_VERSIONS_RELPATH == ".local/share/claude/versions" + assert f'"$HOME/{tools.CLAUDE_VERSIONS_RELPATH}/"' in tools.probe_script() + assert tools.CLAUDE_VERSIONS_RELPATH in tools.transfer_script(fake_payload(tmp_path)) + + +class TestProbeResult: + """The probe's answer is one value with three states, not a boolean.""" + + def test_every_token_the_script_can_print_is_a_state(self): + """The enum's values *are* the wire protocol, so the script and the + parser cannot disagree about what a token means.""" + assert {result.value for result in tools.ProbeResult} == { + "provisioned", + "lendable", + "absent", + } + + @pytest.mark.parametrize( + "answer, expected", + [ + ("provisioned\n", tools.ProbeResult.PROVISIONED), + ("lendable\n", tools.ProbeResult.LENDABLE), + ("absent\n", tools.ProbeResult.ABSENT), + ], + ) + def test_it_reads_the_token_the_probe_printed(self, answer, expected): + assert tools.ProbeResult.parse(answer) is expected + + def test_a_chatty_login_profile_does_not_hide_the_answer(self): + """The probe runs under `bash -lc`, so the container's profile is + sourced first and an image whose profile prints a banner puts that on + stdout ahead of the token. Reading only the last line keeps such an + image on the one-trip path instead of re-provisioning it forever.""" + answer = "Welcome to this image!\n\nprovisioned\n" + assert tools.ProbeResult.parse(answer) is tools.ProbeResult.PROVISIONED + + @pytest.mark.parametrize("answer", ["", "bash: line 1: oh no", "PROVISIONED"]) + def test_an_answer_it_cannot_read_means_absent(self, answer): + """Parsing is total, and it errs towards doing the work again: + provisioning is idempotent, so a wrong `absent` costs a redundant trip + where a wrong `provisioned` would silently skip the whole point.""" + assert tools.ProbeResult.parse(answer) is tools.ProbeResult.ABSENT + + @pytest.mark.usefixtures("no_host_payload") class TestEnsureTools: """The three-trip flow: probe, host transfer, network install.""" def test_a_provisioned_workspace_pays_one_probe_and_nothing_else(self): """The common path: every launch after the first is one round trip.""" - runner = Runner(returncodes=(0,)) + runner = Runner(returncodes=(0,), stdout="provisioned\n") assert ensure_tools("myws", runner) is True assert len(runner.calls) == 1 assert runner.calls[0][:2] == ["ssh", "myws"] # A non-login shell has no ~/.pixi/bin, so every tool would look absent. - assert runner.script().startswith("bash -lc ") - assert "command -v gh" in runner.script() - assert "command -v claude" in runner.script() + argv = shlex.split(runner.script()) + assert argv[:2] == ["bash", "-lc"] + assert argv[2] == tools.probe_script() def test_with_nothing_to_lend_a_cold_workspace_gets_the_network_install(self): - runner = Runner(returncodes=(1, 0)) + runner = Runner(returncodes=(0, 0), stdout="absent\n") assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 # shlex.split is the inverse of the quoting ensure_tools applies. @@ -205,8 +420,17 @@ def test_with_nothing_to_lend_a_cold_workspace_gets_the_network_install(self): assert argv[:2] == ["bash", "-lc"] assert argv[2] == provision_script() + def test_a_probe_trip_that_fails_outright_is_read_as_absent(self): + """The script exits 0 in all three states, so a non-zero trip is the + trip failing rather than an answer -- and the cold flow is the safe + reading of no answer at all.""" + runner = Runner(returncodes=(1, 0), stdout="provisioned\n") + assert ensure_tools("myws", runner) is True + assert len(runner.calls) == 2 + assert shlex.split(runner.script(1))[2] == provision_script() + def test_a_host_with_the_tools_lends_them_instead_of_the_network(self, tmp_path): - runner = Runner(returncodes=(1, 0)) + runner = Runner(returncodes=(0, 0), stdout="absent\n") with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 @@ -217,22 +441,48 @@ def test_a_host_with_the_tools_lends_them_instead_of_the_network(self, tmp_path) def test_a_transfer_the_container_rejects_falls_back_to_the_network(self, tmp_path): """The lent binaries may not run there (arch, libc); the gate at the end of the transfer script reports it, and the old path still runs.""" - runner = Runner(returncodes=(1, 1, 0)) + runner = Runner(returncodes=(0, 1, 0), stdout="absent\n") with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 3 assert shlex.split(runner.script(2))[2] == provision_script() + def test_a_shim_workspace_is_lent_the_hosts_real_claude(self, tmp_path): + """`lendable` is the state this whole ticket exists for: both tools + answer, but the claude is a downloader, so the host replaces it.""" + runner = Runner(returncodes=(0, 0), stdout="lendable\n") + with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): + assert ensure_tools("myws", runner) is True + assert len(runner.calls) == 2 + assert [stream is not None for stream in runner.streams] == [False, True] + assert "tar xf -" in runner.script(1) + + def test_a_shim_the_lend_could_not_replace_is_accepted_not_reinstalled(self, tmp_path): + """The container could not run the lent binaries, but it does have a + claude and a gh. The network fallback decides what to install with its + own `command -v` guards, which both already satisfy, so a third trip + would install nothing -- it is not taken.""" + runner = Runner(returncodes=(0, 1), stdout="lendable\n") + with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): + assert ensure_tools("myws", runner) is True + assert len(runner.calls) == 2 + + def test_a_shim_with_nothing_to_lend_stops_after_the_probe(self): + """Nothing on the host to replace it with, and the network fallback + would no-op, so the shim stands and the launch costs one trip.""" + runner = Runner(returncodes=(0,), stdout="lendable\n") + assert ensure_tools("myws", runner) is True + assert len(runner.calls) == 1 + def test_the_install_output_reaches_the_user_but_the_probe_is_silent(self): """A cold install is tens of seconds; captured, it reads as a hung dl. The scripts' progress lines are the only thing on the terminal during it, so capturing them would be the same as having none. - The probe is the exception: it reports nothing, and its everyday - failure on a cold workspace reaches the terminal as a red devpod - `fatal` describing the probe doing its job. + The probe is the exception: its output is not progress but the answer + the caller branches on, which is exactly what has to be read back. """ - runner = Runner(returncodes=(1, 0)) + runner = Runner(returncodes=(0, 0), stdout="absent\n") ensure_tools("myws", runner) assert runner.captured == [True, False] From ee2b98dd2de9ae91acb3d9f64921df848e783833 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sun, 9 Aug 2026 12:02:12 +0100 Subject: [PATCH 2/3] One definition of the official claude layout, asked from both ends The three-state probe still admitted the illegal state it was written to remove. `CLAUDE_VERSIONS_RELPATH` unified the string but not the relation: the container asked a prefix glob (anything at any depth under the versions directory) while the host asked for a direct child, so a downloader parked at `versions/latest/bin/claude` was trusted by the container and refused by the host on one and the same tree. The mirror of that bug cost the other direction: the glob compared a `readlink -f`-resolved binary against an unresolved `$HOME`, so a genuine install under a symlinked or trailing-slash home read `lendable` on every launch forever -- a ~342MB tar re-paid for the life of the workspace, and the reason the CHANGELOG's "the next launch probes provisioned" was not true. Both are the same defect: two predicates in two languages that have to agree. So there is now one. The container reports the two facts only it can know -- where its `claude` resolves to, and where the versions directory in its own home resolves to -- and `ProbeResult.parse` decides what they mean through `_is_official_claude`, the same relation `_claude_source` asks of the host. The container names no state, so it holds no opinion that can drift from the host's. Folded in while the file was open: `ensure_tools` loses the `tools` parameter it no longer honoured (the probe asks about REQUIRED_TOOLS, so a caller set would be probed for one thing and installed for another), the duplicated `command -v` join becomes `_all_present`, and `$HOME` is expanded defensively so the script really does exit 0 in every state -- an image that never set it used to abort under `set -u`. --- CHANGELOG.md | 37 +++++-- devlaunch/tools.py | 174 +++++++++++++++++++++-------- test/unit/test_tools.py | 239 +++++++++++++++++++++++++++++++--------- 3 files changed, 339 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcf6fff5..410f34d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,15 +96,27 @@ table and should be judged on its own merits. any built from this repo's own `.devcontainer/claude-code/` feature) skipped the lend and paid the ~285MB GCS download on first use, the exact cost the lending exists to remove. It now answers one of three states: `provisioned` - when `gh` is on the login `PATH` **and** `claude` resolves into - `~/.local/share/claude/versions/` — the same definition of "a real claude" - the host already applies to decide what it may lend, now written down once - and used on both sides of the pipe; `lendable` when both answer but the - claude is a shim or wrapper; `absent` otherwise. - - A `lendable` container is quietly upgraded: the host streams in its own - binary and the transfer's `~/.local/bin` `PATH` prepend is what makes it win - from then on, so the *next* launch probes `provisioned`. If the host has + when `gh` is on the login `PATH` **and** `claude` resolves to a binary the + official installer itself put in `~/.local/share/claude/versions/`; + `lendable` when both answer but the claude is a shim or wrapper; `absent` + otherwise. + + "The official install" is one definition, asked from both ends of the pipe. + The container reports two facts only it can know — where its `claude` + resolves to, and where that directory in its own home resolves to — and + which state those mean is decided on the host, by the same code that decides + what the host may lend. Two copies of that rule, one per language, is what a + shared constant alone does not prevent: a downloader parked at + `versions/latest/bin/claude` satisfies "somewhere under the versions + directory" while failing "a binary the installer wrote", and a probe holding + the looser of the two opinions trusts it. + + Both paths are compared fully resolved, which is what makes the upgrade + terminate on an image whose `$HOME` is reached through a symlink: a + `lendable` container is quietly upgraded — the host streams in its own binary + and the transfer's `~/.local/bin` `PATH` prepend is what makes it win from + then on — so the *next* launch probes `provisioned` and the tar is paid once + rather than on every launch for the life of the workspace. If the host has nothing to lend, or the lent binaries do not run there, the container is accepted as it stands rather than falling through to the network install — that install decides what to do with its own `command -v` guards, which a @@ -112,9 +124,10 @@ table and should be judged on its own merits. The probe never executes the candidate `claude`; on a shim, *any* invocation triggers the very download the probe exists to detect. It resolves the path - instead. It also exits 0 in all three states now, which retires the red - devpod `fatal ... Process exited with status 1` that the old probe's - everyday cold answer painted on the terminal. + instead. It also exits 0 in every state now — including in an image that + never set `HOME` — which retires the red devpod + `fatal ... Process exited with status 1` that the old probe's everyday cold + answer painted on the terminal. Two things this deliberately does not do. It does not compare versions: a real `claude` already in the container is left alone however old it is, since diff --git a/devlaunch/tools.py b/devlaunch/tools.py index 08248ad7..7eefe349 100644 --- a/devlaunch/tools.py +++ b/devlaunch/tools.py @@ -28,10 +28,12 @@ than two, because a `claude` on PATH is not necessarily a `claude` worth keeping: the shim this repo's own devcontainer feature bakes satisfies `command -v` while still owing the GCS download on first use. So the probe -reports `provisioned` only for the official install layout, `lendable` for a +answers `provisioned` only for the official install layout, `lendable` for a container whose claude is something else, and `absent` when a tool is really missing -- and a lendable container is quietly upgraded to the host's real -binary the next time it goes through `up`. +binary the next time it goes through `up`. The container reports what it +found and the host says what that means, so "the official install layout" is +one definition asked from both ends rather than one per side. The round trips this costs, by path: a provisioned workspace pays one (the probe, which was always paid); a lendable or cold one pays two (probe, then @@ -73,13 +75,18 @@ BLOOOP_CHANNEL = "https://prefix.dev/blooop" # Where the official claude installer keeps one binary per version, relative to -# a home directory. This is the definition of "a real claude" on both sides of -# the pipe -- the host reads it to decide what it may lend (`_claude_source`), -# writes into it when it lends (`transfer_script`), and the container is asked -# about it to decide whether it needs anything (`probe_script`). Stated once, -# because two copies of that definition are two probes with different opinions. +# a home directory. The host reads it to decide what it may lend +# (`_claude_source`), writes into it when it lends (`transfer_script`), and the +# container is asked about it to decide whether it needs anything +# (`probe_script`). The *relation* that makes a path the official layout lives +# in `_is_official_claude`, for the same reason this string lives here: two +# copies of a definition are two probes with different opinions. CLAUDE_VERSIONS_RELPATH = ".local/share/claude/versions" +# What every line of a probe's report starts with, so the report survives being +# printed into the same stdout as a container login profile's banner. +PROBE_MARK = "devlaunch-probe" + # Which file a bash login shell will actually read. bash tries ~/.bash_profile, # ~/.bash_login and ~/.profile in that order and sources only the first that # exists, so appending to ~/.profile in an image that ships a ~/.bash_profile @@ -126,6 +133,41 @@ def provisioning_disabled() -> bool: return os.environ.get(DISABLE_VAR, "").strip().lower() not in _FALSEY +def _all_present(tools: Sequence[Tool]) -> str: + """The bash test for "every one of these already answers on PATH".""" + return " && ".join(f"command -v {shlex.quote(tool.command)} >/dev/null 2>&1" for tool in tools) + + +def _is_official_claude(versions_dir: str, claude_binary: str) -> bool: + """Whether a resolved `claude` is a binary of the official install. + + The one definition of "the official layout", and both sides of the pipe + ask it: the host asks it of its own filesystem to decide what it may lend, + and the container's probe reports its two resolved paths so it can be asked + of those here. Written down once because a container-side copy of this + relation and a host-side copy are two probes with different opinions -- + which is how a downloader parked at `versions/latest/bin/claude` came to be + trusted by one while the other refused to lend the very same tree. + + A *direct child*, because that is what the installer creates: one binary + per version, named for the version. Anything deeper is somebody else's + tree that merely starts with the official path, and a downloader is free + to choose such a path. + + Both arguments must arrive resolved -- `readlink -f` in the container, + `Path.resolve` on the host -- because either home may be reached through a + symlink, and comparing a resolved binary against an unresolved home makes + a genuine install look like a shim forever. Anything that is not an + absolute path (the empty answer a container with no `readlink` gives, a + truncated line) is not the official layout: of the two ways to be wrong, + lending again costs seconds and trusting a shim ships a container that + still owes the download. + """ + if not versions_dir.startswith("/") or not claude_binary.startswith("/"): + return False + return pathlib.PurePosixPath(claude_binary).parent == pathlib.PurePosixPath(versions_dir) + + def _install_line(tool: Tool) -> str: args = " ".join(shlex.quote(arg) for arg in tool.install_args) return ( @@ -148,9 +190,7 @@ def provision_script(tools: Sequence[Tool] = REQUIRED_TOOLS) -> str: Exits 0 unless an install actually failed, so "nothing to do" and "all installs worked" are the same answer to the caller. """ - all_present = " && ".join( - f"command -v {shlex.quote(tool.command)} >/dev/null 2>&1" for tool in tools - ) + all_present = _all_present(tools) installs = "\n".join(_install_line(tool) for tool in tools) # The trampoline pixi writes into ~/.pixi/bin does not work for packages # that ship a shell script, which is why the env's own bin directory is @@ -203,8 +243,9 @@ class ProbeResult(enum.Enum): A boolean plus a flag would have been the same two questions with a fourth, meaningless combination available; three named states have no illegal one. - The values are the tokens `probe_script` prints, so the protocol over the - ssh channel is written down once rather than once per side. + Which state a report means is decided here and nowhere else. The container + reports what it found and never names a state, so there is no container- + side copy of "the official layout" to drift away from the host's. """ PROVISIONED = "provisioned" @@ -212,49 +253,77 @@ class ProbeResult(enum.Enum): ABSENT = "absent" @classmethod - def parse(cls, answer: str) -> "ProbeResult": - """Read a probe's answer. Total: anything unrecognised is ABSENT. + def parse(cls, report: str) -> "ProbeResult": + """Read a probe's report. Total: anything unreadable is ABSENT. + + The report is marked `key value` lines rather than one token because a + token would mean the container had already decided -- with its own copy + of the relation `_is_official_claude` states once, for both sides. + What crosses the pipe is therefore two resolved paths only the + container can know, and what they mean is settled here. - The last line, not the whole output: the probe runs under `bash -lc`, - so the container's login profile is sourced before the script and an - image whose profile prints a banner puts it on stdout ahead of the - token. The token is always the last thing printed. + Marked lines, and read from anywhere in the output, because the probe + runs under `bash -lc`: the container's login profile is sourced first, + so an image whose profile prints a banner puts it on the same stdout. - A garbled, empty or truncated answer has to mean something, and ABSENT + A garbled, empty or truncated report has to mean something, and ABSENT is the only state whose worst case is harmless -- provisioning is idempotent, so reading it wrongly costs a redundant round trip, where a wrong PROVISIONED would silently skip the work the probe exists to schedule. """ - lines = [line.strip() for line in answer.splitlines() if line.strip()] - if not lines: - return cls.ABSENT - try: - return cls(lines[-1]) - except ValueError: + found = {} + for line in report.splitlines(): + mark, _, rest = line.strip().partition(" ") + if mark != PROBE_MARK: + continue + key, _, value = rest.partition(" ") + found[key] = value.strip() + if found.get("tools") != "present": return cls.ABSENT + if _is_official_claude(found.get("versions", ""), found.get("claude", "")): + return cls.PROVISIONED + return cls.LENDABLE def probe_script() -> str: - """The shell script that answers what provisioning a workspace still needs. - - Prints exactly one token -- `provisioned`, `lendable` or `absent` -- and - exits 0 in every one of them: "this container has nothing" is an answer, - not a failure, and a probe that exits non-zero paints a red devpod + """The shell script that reports what a workspace already has. + + It reports; it does not decide. Whether a container counts as provisioned + turns on two facts only the container can know -- where its `claude` + resolves to, and where the official versions directory in its home + resolves to -- so those are what it prints, and `ProbeResult.parse` says + what they mean. That split is the point: the relation between the two + paths is stated once, in `_is_official_claude`, instead of once per + language, which is what let a shim be trusted by one side and refused by + the other. + + Exits 0 in every state: "this container has nothing" is an answer, not a + failure, and a probe that exits non-zero paints a red devpod `fatal ... Process exited with status 1` on the terminal of every cold - launch, describing the probe working exactly as intended. + launch, describing the probe working exactly as intended. Nothing here can + fail -- `$HOME` is expanded defensively because an image that never set it + would otherwise abort the script under `set -u`, and a path that will not + resolve is reported empty, which reads as `lendable`. It asks about the fixed pair this module lends rather than an arbitrary tool set, for the same reason `host_payload` does: what can be lent is `gh` and `claude`, so those are what there is a three-state answer for. """ - present = " && ".join( - f"command -v {shlex.quote(tool.command)} >/dev/null 2>&1" for tool in REQUIRED_TOOLS - ) return "\n".join( [ "set -u", - f"if ! {{ {present} ; }}; then echo {ProbeResult.ABSENT.value}; exit 0; fi", + f"if ! {{ {_all_present(REQUIRED_TOOLS)} ; }}; then", + f' echo "{PROBE_MARK} tools missing"', + " exit 0", + "fi", + f'echo "{PROBE_MARK} tools present"', + # Both paths fully resolved, because the comparison they are for is + # an equality: a home reached through a symlink resolves one side + # and not the other, and a real install then reads `lendable` on + # every launch forever. + f'echo "{PROBE_MARK} versions ' + f'$(readlink -f "${{HOME-}}/{CLAUDE_VERSIONS_RELPATH}" 2>/dev/null || true)"', # Resolved, never run. The shim on PATH answers `command -v` just # as the real binary does, and telling them apart by asking either # one for its version would trigger, on the shim, the very ~285MB @@ -262,11 +331,7 @@ def probe_script() -> str: # is the whole test. `readlink` failing (or missing) leaves this # empty, which reads as `lendable` -- the answer that does more # work, never the one that skips it. - 'target=$(readlink -f "$(command -v claude)" 2>/dev/null || true)', - 'case "$target" in', - f' "$HOME/{CLAUDE_VERSIONS_RELPATH}/"*) echo {ProbeResult.PROVISIONED.value} ;;', - f" *) echo {ProbeResult.LENDABLE.value} ;;", - "esac", + f'echo "{PROBE_MARK} claude $(readlink -f "$(command -v claude)" 2>/dev/null || true)"', ] ) @@ -311,16 +376,23 @@ def _claude_source() -> Optional[Tuple[str, pathlib.Path]]: The official installer keeps one binary per version under `~/.local/share/claude/versions/` and points `~/.local/bin/claude` at the current one. Anything else on PATH answering to `claude` -- the pixi shim, - a wrapper script -- is exactly the kind of downloader this transfer exists - to skip, so only the official layout counts. + a wrapper script, a downloader that parked itself deeper inside that same + directory -- is exactly the kind of downloader this transfer exists to + skip, so only the official layout counts, and what counts as the official + layout is `_is_official_claude`: the same relation the container's report + is read through, so the two sides cannot come to different answers about + one tree. """ - link = pathlib.Path.home() / ".local/bin/claude" + home = pathlib.Path.home() + link = home / ".local/bin/claude" try: target = link.resolve(strict=True) except OSError: return None - versions_dir = (pathlib.Path.home() / CLAUDE_VERSIONS_RELPATH).resolve() - if target.parent != versions_dir or not os.access(target, os.X_OK): + versions_dir = (home / CLAUDE_VERSIONS_RELPATH).resolve() + if not _is_official_claude(str(versions_dir), str(target)): + return None + if not os.access(target, os.X_OK): return None return target.name, target @@ -480,8 +552,12 @@ def _transfer(workspace: str, runner, payload: HostPayload) -> bool: return result.returncode == 0 -def ensure_tools(workspace: str, runner, tools: Sequence[Tool] = REQUIRED_TOOLS) -> bool: - """Make `tools` available in `workspace`. Returns whether they now are. +def ensure_tools(workspace: str, runner) -> bool: + """Make REQUIRED_TOOLS available in `workspace`. Returns whether they now are. + + Not parametric over a tool set, because the probe is not: it asks about the + fixed pair this module can lend, so a caller-supplied set would be probed + for one thing and installed for another. `runner` is dl.run_devpod, passed in rather than imported to keep this module off dl's import cycle and testable without a devpod. @@ -545,7 +621,7 @@ def ensure_tools(workspace: str, runner, tools: Sequence[Tool] = REQUIRED_TOOLS) return True # ProbeResult.ABSENT: the cold flow, with a tool genuinely missing. - script = provision_script(tools) + script = provision_script() result = runner(["ssh", workspace, "--command", f"bash -lc {shlex.quote(script)}"]) except OSError as e: logging.debug("Could not install tools into %s: %s", workspace, e) @@ -556,7 +632,7 @@ def ensure_tools(workspace: str, runner, tools: Sequence[Tool] = REQUIRED_TOOLS) # session, not for an install. logging.warning( "Could not install %s into %s; the session will start without them.", - " and ".join(tool.command for tool in tools), + " and ".join(tool.command for tool in REQUIRED_TOOLS), workspace, ) return False diff --git a/test/unit/test_tools.py b/test/unit/test_tools.py index c29258e3..6e8d79b3 100644 --- a/test/unit/test_tools.py +++ b/test/unit/test_tools.py @@ -183,6 +183,23 @@ def fake_payload(tmp_path) -> tools.HostPayload: ) +# What a container in each of the three states reports back over the pipe, +# written out as the probe prints it rather than rebuilt from the module: a +# fixture composed the way the parser splits lines would agree with any format +# the probe ever drifted into. `/ws` stands in for a container's $HOME. +REPORT_ABSENT = "devlaunch-probe tools missing\n" +REPORT_PROVISIONED = ( + "devlaunch-probe tools present\n" + "devlaunch-probe versions /ws/.local/share/claude/versions\n" + "devlaunch-probe claude /ws/.local/share/claude/versions/2.0.1\n" +) +REPORT_LENDABLE = ( + "devlaunch-probe tools present\n" + "devlaunch-probe versions /ws/.local/share/claude/versions\n" + "devlaunch-probe claude /ws/.pixi/envs/claude-shim/bin/claude\n" +) + + class TestProbeScript: """The probe runs in someone else's container and decides whether to lend. @@ -213,12 +230,22 @@ def _bare_path(tmp_path): (sysbin / command).symlink_to(found) return [sysbin] - def _answer(self, tmp_path, home, path_dirs=()) -> str: - """Run the probe script for real and return the single token it printed.""" - env = { - "HOME": str(home), - "PATH": ":".join(str(d) for d in [*path_dirs, *self._bare_path(tmp_path)]), - } + _KEEP_HOME = object() + + def _answer(self, tmp_path, home, path_dirs=(), *, home_env=_KEEP_HOME): + """Run the probe for real and read its answer the way `dl` reads it. + + The script and the parser together, because that pair *is* the probe: + the container reports what it found and this side says what that means, + so a test of either half alone would not notice the two disagreeing. + + `home_env` replaces what `$HOME` is set to for the run, or removes the + variable entirely when it is None. + """ + env = {"PATH": ":".join(str(d) for d in [*path_dirs, *self._bare_path(tmp_path)])} + home_value = str(home) if home_env is self._KEEP_HOME else home_env + if home_value is not None: + env["HOME"] = home_value # bash by absolute path: the stripped PATH the probe is handed cannot # also be the one that finds the shell running it. result = subprocess.run( @@ -232,7 +259,7 @@ def _answer(self, tmp_path, home, path_dirs=()) -> str: # and a non-zero probe paints a red devpod `fatal` on the terminal of # every cold launch. assert result.returncode == 0, result.stderr - return result.stdout.strip() + return tools.ProbeResult.parse(result.stdout) @staticmethod def _official_claude(home, version="2.0.1"): @@ -268,13 +295,13 @@ def _gh(home): def test_a_container_with_neither_tool_answers_absent(self, tmp_path): home = self._home(tmp_path) - assert self._answer(tmp_path, home) == "absent" + assert self._answer(tmp_path, home) is tools.ProbeResult.ABSENT def test_the_official_claude_layout_answers_provisioned(self, tmp_path): """The prebuilt-image jackpot: nothing to do, one round trip.""" home = self._home(tmp_path) self._official_claude(home) - assert self._answer(tmp_path, home, [self._gh(home)]) == "provisioned" + assert self._answer(tmp_path, home, [self._gh(home)]) is tools.ProbeResult.PROVISIONED def test_a_baked_shim_answers_lendable_rather_than_provisioned(self, tmp_path): """The whole point of the three states. A `claude` that is a downloader @@ -282,14 +309,14 @@ def test_a_baked_shim_answers_lendable_rather_than_provisioned(self, tmp_path): to avoid, so it must not be mistaken for a provisioned workspace.""" home = self._home(tmp_path) self._claude_shim(home) - assert self._answer(tmp_path, home, [self._gh(home)]) == "lendable" + assert self._answer(tmp_path, home, [self._gh(home)]) is tools.ProbeResult.LENDABLE def test_a_real_claude_with_no_gh_is_absent_not_lendable(self, tmp_path): """`lendable` means "replace the claude"; a container missing gh outright needs the cold flow, which the network fallback can finish.""" home = self._home(tmp_path) self._official_claude(home) - assert self._answer(tmp_path, home, [home / ".local/bin"]) == "absent" + assert self._answer(tmp_path, home, [home / ".local/bin"]) is tools.ProbeResult.ABSENT def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): """Convergence, run for real end to end: the transfer script unpacks @@ -329,7 +356,90 @@ def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): check=False, ) assert lend.returncode == 0, lend.stderr - assert self._answer(tmp_path, home, [home / ".local/bin"]) == "provisioned" + assert self._answer(tmp_path, home, [home / ".local/bin"]) is tools.ProbeResult.PROVISIONED + + @staticmethod + def _nested_shim(home): + """A downloader that parked itself *inside* the versions directory. + + The official installer puts one binary per version directly in that + directory; anything deeper is somebody else's tree, and a downloader + is free to choose a path that merely starts with the official one. + """ + shim = home / ".local/share/claude/versions/latest/bin/claude" + shim.parent.mkdir(parents=True, exist_ok=True) + shim.write_text("#!/bin/sh\necho 'downloading 285MB' >&2\n", encoding="utf-8") + shim.chmod(0o755) + link = home / ".local/bin/claude" + link.parent.mkdir(parents=True, exist_ok=True) + link.symlink_to(shim) + return link.parent + + def test_a_shim_hiding_inside_the_versions_directory_is_lendable(self, tmp_path): + """`under the versions directory` is not the official layout -- being a + direct child of it is. A downloader is free to park itself at + `versions/latest/bin/claude`, and a probe that accepts any depth trusts + it and leaves the container owing the download the lend exists to + remove -- the exact illegal state the three-state probe replaced.""" + home = self._home(tmp_path) + self._nested_shim(home) + assert self._answer(tmp_path, home, [self._gh(home)]) is tools.ProbeResult.LENDABLE + + @pytest.mark.parametrize("layout", ["_official_claude", "_claude_shim", "_nested_shim"]) + def test_neither_side_of_the_pipe_can_disagree_about_one_tree( + self, tmp_path, monkeypatch, layout + ): + """One definition of "the official install", asked from both ends. + + The container decides whether to keep what it has; the host decides + what it may lend. Those are the same question about the same layout, + and an answer of `provisioned` for a tree the host would refuse to lend + is a shim nothing will ever replace -- so the two are pinned together + rather than separately. + """ + home = self._home(tmp_path) + getattr(self, layout)(home) + self._gh(home) + container = self._answer(tmp_path, home, [home / ".local/bin"]) + monkeypatch.setattr(tools.pathlib.Path, "home", staticmethod(lambda: home)) + host = tools._claude_source() # pylint: disable=protected-access + assert (container is tools.ProbeResult.PROVISIONED) is (host is not None) + + def test_a_real_install_reached_through_a_symlinked_home_is_provisioned(self, tmp_path): + """Images reach `$HOME` through a symlink often enough to matter, and + resolving the claude while comparing against an unresolved `$HOME` + makes a genuine install read `lendable` forever: the lend succeeds, + changes nothing the next probe recognises, and is re-paid on every + single `up` for the life of the workspace.""" + real = tmp_path / "real-home" + real.mkdir() + self._official_claude(real) + self._gh(real) + home = tmp_path / "home-link" + home.symlink_to(real) + answer = self._answer(tmp_path, home, [home / ".local/bin"]) + assert answer is tools.ProbeResult.PROVISIONED + + def test_a_trailing_slash_on_home_does_not_hide_the_official_install(self, tmp_path): + """A `$HOME` written with a trailing slash is a legal value of the + variable and names the same directory, so it must not turn a real + install into a lend.""" + home = self._home(tmp_path) + self._official_claude(home) + self._gh(home) + answer = self._answer(tmp_path, home, [home / ".local/bin"], home_env=f"{home}/") + assert answer is tools.ProbeResult.PROVISIONED + + def test_a_container_with_no_home_set_still_answers(self, tmp_path): + """`exits 0 in every state` has to survive an image that never set + `HOME`: under `set -u` an unguarded expansion aborts the script, which + is the red devpod `fatal` this probe was written to retire.""" + home = self._home(tmp_path) + self._official_claude(home) + assert ( + self._answer(tmp_path, home, [self._gh(home)], home_env=None) + is tools.ProbeResult.LENDABLE + ) def test_it_resolves_the_link_rather_than_trusting_the_path_entry(self): """In the official layout `~/.local/bin/claude` is a symlink, so what @@ -343,57 +453,86 @@ def test_it_never_runs_the_candidate_claude(self): path up and resolve it. """ script = tools.probe_script() - scrubbed = script.replace("command -v claude", "").replace( - f'"$HOME/{tools.CLAUDE_VERSIONS_RELPATH}/"', "" + scrubbed = ( + script.replace("command -v claude", "") + .replace(tools.CLAUDE_VERSIONS_RELPATH, "") + .replace("devlaunch-probe claude", "") ) assert "claude" not in scrubbed assert "--version" not in script + def test_the_container_is_never_asked_to_name_a_state(self): + """The container reports two resolved paths and no verdict. + + A token would mean it had decided, and deciding needs its own copy of + "the official layout" -- the second opinion that let a shim parked + deeper under the versions directory be trusted by the container while + the host refused to lend the very same tree. + """ + script = tools.probe_script() + for state in tools.ProbeResult: + assert state.value not in script + def test_the_official_layout_is_defined_once_for_both_sides_of_the_pipe(self, tmp_path): """The container-side "is this the official install" and the host-side - one it mirrors must not be able to drift apart.""" + one it mirrors must not be able to drift apart, so there is one of it: + both go through `_is_official_claude`, and the constant it is asked + about is likewise stated once.""" assert tools.CLAUDE_VERSIONS_RELPATH == ".local/share/claude/versions" - assert f'"$HOME/{tools.CLAUDE_VERSIONS_RELPATH}/"' in tools.probe_script() + assert tools.CLAUDE_VERSIONS_RELPATH in tools.probe_script() assert tools.CLAUDE_VERSIONS_RELPATH in tools.transfer_script(fake_payload(tmp_path)) class TestProbeResult: """The probe's answer is one value with three states, not a boolean.""" - def test_every_token_the_script_can_print_is_a_state(self): - """The enum's values *are* the wire protocol, so the script and the - parser cannot disagree about what a token means.""" - assert {result.value for result in tools.ProbeResult} == { - "provisioned", - "lendable", - "absent", - } + def test_it_reads_the_state_out_of_what_the_container_reported(self): + assert tools.ProbeResult.parse(REPORT_ABSENT) is tools.ProbeResult.ABSENT + assert tools.ProbeResult.parse(REPORT_PROVISIONED) is tools.ProbeResult.PROVISIONED + assert tools.ProbeResult.parse(REPORT_LENDABLE) is tools.ProbeResult.LENDABLE + + def test_a_claude_deeper_under_the_versions_directory_is_not_the_install(self): + """`under` is not `in`. The installer writes one binary per version + directly into that directory, so a path that merely starts with it is + somebody else's -- a downloader's, in the case this ticket exists for.""" + report = ( + "devlaunch-probe tools present\n" + "devlaunch-probe versions /ws/.local/share/claude/versions\n" + "devlaunch-probe claude /ws/.local/share/claude/versions/latest/bin/claude\n" + ) + assert tools.ProbeResult.parse(report) is tools.ProbeResult.LENDABLE - @pytest.mark.parametrize( - "answer, expected", - [ - ("provisioned\n", tools.ProbeResult.PROVISIONED), - ("lendable\n", tools.ProbeResult.LENDABLE), - ("absent\n", tools.ProbeResult.ABSENT), - ], - ) - def test_it_reads_the_token_the_probe_printed(self, answer, expected): - assert tools.ProbeResult.parse(answer) is expected + def test_a_container_that_could_resolve_nothing_is_not_provisioned(self): + """Two blanks are equal, and equality is the whole test -- so a + container with no `readlink`, which resolves neither path, must not + come out as the one perfect match.""" + report = "devlaunch-probe tools present\ndevlaunch-probe versions\ndevlaunch-probe claude\n" + assert tools.ProbeResult.parse(report) is tools.ProbeResult.LENDABLE def test_a_chatty_login_profile_does_not_hide_the_answer(self): """The probe runs under `bash -lc`, so the container's profile is sourced first and an image whose profile prints a banner puts that on - stdout ahead of the token. Reading only the last line keeps such an - image on the one-trip path instead of re-provisioning it forever.""" - answer = "Welcome to this image!\n\nprovisioned\n" - assert tools.ProbeResult.parse(answer) is tools.ProbeResult.PROVISIONED + the same stdout. The marked lines are what the report is, which keeps + such an image on the one-trip path instead of re-provisioning it + forever.""" + report = "Welcome to this image!\n\n" + REPORT_PROVISIONED + assert tools.ProbeResult.parse(report) is tools.ProbeResult.PROVISIONED - @pytest.mark.parametrize("answer", ["", "bash: line 1: oh no", "PROVISIONED"]) - def test_an_answer_it_cannot_read_means_absent(self, answer): + @pytest.mark.parametrize( + "report", + [ + "", + "bash: line 1: oh no", + "provisioned", + "devlaunch-probe tools\n", + "tools present\nversions /ws/.local/share/claude/versions\n", + ], + ) + def test_a_report_it_cannot_read_means_absent(self, report): """Parsing is total, and it errs towards doing the work again: provisioning is idempotent, so a wrong `absent` costs a redundant trip where a wrong `provisioned` would silently skip the whole point.""" - assert tools.ProbeResult.parse(answer) is tools.ProbeResult.ABSENT + assert tools.ProbeResult.parse(report) is tools.ProbeResult.ABSENT @pytest.mark.usefixtures("no_host_payload") @@ -402,7 +541,7 @@ class TestEnsureTools: def test_a_provisioned_workspace_pays_one_probe_and_nothing_else(self): """The common path: every launch after the first is one round trip.""" - runner = Runner(returncodes=(0,), stdout="provisioned\n") + runner = Runner(returncodes=(0,), stdout=REPORT_PROVISIONED) assert ensure_tools("myws", runner) is True assert len(runner.calls) == 1 assert runner.calls[0][:2] == ["ssh", "myws"] @@ -412,7 +551,7 @@ def test_a_provisioned_workspace_pays_one_probe_and_nothing_else(self): assert argv[2] == tools.probe_script() def test_with_nothing_to_lend_a_cold_workspace_gets_the_network_install(self): - runner = Runner(returncodes=(0, 0), stdout="absent\n") + runner = Runner(returncodes=(0, 0), stdout=REPORT_ABSENT) assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 # shlex.split is the inverse of the quoting ensure_tools applies. @@ -424,13 +563,13 @@ def test_a_probe_trip_that_fails_outright_is_read_as_absent(self): """The script exits 0 in all three states, so a non-zero trip is the trip failing rather than an answer -- and the cold flow is the safe reading of no answer at all.""" - runner = Runner(returncodes=(1, 0), stdout="provisioned\n") + runner = Runner(returncodes=(1, 0), stdout=REPORT_PROVISIONED) assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 assert shlex.split(runner.script(1))[2] == provision_script() def test_a_host_with_the_tools_lends_them_instead_of_the_network(self, tmp_path): - runner = Runner(returncodes=(0, 0), stdout="absent\n") + runner = Runner(returncodes=(0, 0), stdout=REPORT_ABSENT) with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 @@ -441,7 +580,7 @@ def test_a_host_with_the_tools_lends_them_instead_of_the_network(self, tmp_path) def test_a_transfer_the_container_rejects_falls_back_to_the_network(self, tmp_path): """The lent binaries may not run there (arch, libc); the gate at the end of the transfer script reports it, and the old path still runs.""" - runner = Runner(returncodes=(0, 1, 0), stdout="absent\n") + runner = Runner(returncodes=(0, 1, 0), stdout=REPORT_ABSENT) with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 3 @@ -450,7 +589,7 @@ def test_a_transfer_the_container_rejects_falls_back_to_the_network(self, tmp_pa def test_a_shim_workspace_is_lent_the_hosts_real_claude(self, tmp_path): """`lendable` is the state this whole ticket exists for: both tools answer, but the claude is a downloader, so the host replaces it.""" - runner = Runner(returncodes=(0, 0), stdout="lendable\n") + runner = Runner(returncodes=(0, 0), stdout=REPORT_LENDABLE) with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 @@ -462,7 +601,7 @@ def test_a_shim_the_lend_could_not_replace_is_accepted_not_reinstalled(self, tmp claude and a gh. The network fallback decides what to install with its own `command -v` guards, which both already satisfy, so a third trip would install nothing -- it is not taken.""" - runner = Runner(returncodes=(0, 1), stdout="lendable\n") + runner = Runner(returncodes=(0, 1), stdout=REPORT_LENDABLE) with patch.object(tools, "host_payload", return_value=fake_payload(tmp_path)): assert ensure_tools("myws", runner) is True assert len(runner.calls) == 2 @@ -470,7 +609,7 @@ def test_a_shim_the_lend_could_not_replace_is_accepted_not_reinstalled(self, tmp def test_a_shim_with_nothing_to_lend_stops_after_the_probe(self): """Nothing on the host to replace it with, and the network fallback would no-op, so the shim stands and the launch costs one trip.""" - runner = Runner(returncodes=(0,), stdout="lendable\n") + runner = Runner(returncodes=(0,), stdout=REPORT_LENDABLE) assert ensure_tools("myws", runner) is True assert len(runner.calls) == 1 @@ -482,7 +621,7 @@ def test_the_install_output_reaches_the_user_but_the_probe_is_silent(self): The probe is the exception: its output is not progress but the answer the caller branches on, which is exactly what has to be read back. """ - runner = Runner(returncodes=(0, 0), stdout="absent\n") + runner = Runner(returncodes=(0, 0), stdout=REPORT_ABSENT) ensure_tools("myws", runner) assert runner.captured == [True, False] From 1c02df4b3544e4b086becce65cf09c0d4fddf52b Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sun, 9 Aug 2026 12:31:27 +0100 Subject: [PATCH 3/3] Ask the profile whether devlaunch already edited it, not whether Ubuntu did A lend only takes effect because transfer_script prepends ~/.local/bin to the container's login profile, putting the binary just lent in front of the shim that was already there. The guard in front of that append asked whether ".local/bin" appeared anywhere in the profile -- and Ubuntu's stock ~/.profile prepends ~/.local/bin itself, near the top of the file. So on mcr.microsoft.com/devcontainers/base:ubuntu-24.04, the image this repo's own devcontainer builds on, the guard read the image's block as devlaunch's own work and skipped the append. The shim stayed first, the next probe answered `lendable` again, and the whole multi-hundred-megabyte transfer was re-paid on every `devpod up` for the life of the workspace. Every PATH line these scripts append now goes in under a `# devlaunch:` mark, and every guard is an exact-line fixed-string match on that mark -- a line only devlaunch ever writes. Applied to all three guards, not just the one that was biting: the two in provision_script had the same shape, matching ".pixi/bin" and "pixi/envs/claude-shim" in a file they do not own. A profile some older devlaunch already edited gains one duplicate PATH entry the first time it is seen, and nothing after. The convergence test could not see any of this, because it handed the probe a PATH instead of letting the profile build one -- which asserts a world where the profile edit cannot be wrong. It now sources the profile, against a $HOME seeded with that image's stock file plus the lines this repo's devcontainer appends, and asserts the answer after a real lend. Also drops the inherited ~5s figure from the accepted-residual comment. It was never measured; #158 measured the staging step at ~0.13s warm and ~2.6s cold and found ~5.1s is not a staging cost. --- CHANGELOG.md | 22 ++++- devlaunch/tools.py | 78 ++++++++++++--- test/unit/test_tools.py | 208 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 280 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 410f34d2..5df2583f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -116,7 +116,27 @@ table and should be judged on its own merits. `lendable` container is quietly upgraded — the host streams in its own binary and the transfer's `~/.local/bin` `PATH` prepend is what makes it win from then on — so the *next* launch probes `provisioned` and the tar is paid once - rather than on every launch for the life of the workspace. If the host has + rather than on every launch for the life of the workspace. + + That prepend only actually happens because the guard in front of it now asks + the right question. **Every line devlaunch appends to a container's login + profile is written under a `# devlaunch:` mark, and the "have I already done + this?" guard is an exact match on that mark** — not, as before, a search for + the directory being added. Searching for the directory made the answer a base + image's to give, and it gave the wrong one: Ubuntu's stock `~/.profile` + prepends `~/.local/bin` itself, near the top of the file, so on + `mcr.microsoft.com/devcontainers/base:ubuntu-24.04` — the image this repo's + own devcontainer builds on — the transfer read that block as its own work, + skipped the prepend, and left the shim ahead of the binary it had just lent. + The workspace never converged: it answered `lendable`, re-paid the whole + transfer, and answered `lendable` again, every `devpod up`, forever. The + convergence test now lets the profile decide `PATH` — sourced from a home + seeded with that image's stock file plus the lines this repo's devcontainer + appends — because a test that builds `PATH` itself cannot see the ordering + the lend depends on. A profile some earlier devlaunch already edited gains + one duplicate `PATH` entry the first time it is seen, and nothing after that. + + If the host has nothing to lend, or the lent binaries do not run there, the container is accepted as it stands rather than falling through to the network install — that install decides what to do with its own `command -v` guards, which a diff --git a/devlaunch/tools.py b/devlaunch/tools.py index 7eefe349..5843891a 100644 --- a/devlaunch/tools.py +++ b/devlaunch/tools.py @@ -100,6 +100,20 @@ ] ) +# What devlaunch writes above every PATH line it appends to a container's login +# profile, and the only thing its "have I already done this?" guard looks for. +# The mark exists so the guard can ask about devlaunch's own work instead of +# about a directory name. Asking about the directory made the answer depend on +# a file devlaunch does not own: Ubuntu's stock ~/.profile prepends +# ~/.local/bin itself, so on this repo's own base image +# (mcr.microsoft.com/devcontainers/base:ubuntu-24.04) the transfer's guard read +# the image's block as its own, skipped the prepend, and left the claude shim +# in front of the binary just lent -- which made every `devpod up` re-pay a +# multi-hundred-megabyte transfer for the life of the workspace. The same +# lesson devpod_provider learned from grepping devpod's rendered table: a guard +# that reads someone else's artifact is only correct until they change it. +PROFILE_MARK = "# devlaunch:" + @dataclass(frozen=True) class Tool: @@ -168,6 +182,33 @@ def _is_official_claude(versions_dir: str, claude_binary: str) -> bool: return pathlib.PurePosixPath(claude_binary).parent == pathlib.PurePosixPath(versions_dir) +def _profile_prepend(tag: str, line: str, on_failure: str = "") -> str: + """One PATH line appended to `$PROFILE` at most once, ever. + + The line is written under a `PROFILE_MARK` comment naming `tag`, and the + guard is an exact-line match on that comment -- so what decides whether the + edit has already been made is a line only these scripts ever write. + Substring-matching the directory being added cannot do that job: a base + image is free to mention, or even prepend, the same directory for its own + reasons, and then the guard skips an append the workspace still needs. + + Exact-line (`-x`) and fixed-string (`-F`) rather than a pattern, so nothing + in the mark is read as a regex and a longer line that merely contains it + does not count as a match. + + Appending under a *new* mark to a profile some older devlaunch already + edited duplicates one PATH entry, once: harmless (a directory twice on PATH + resolves the same), self-limiting (the mark is there from then on), and the + price of the guard no longer being able to lie. + """ + mark = f"{PROFILE_MARK} {tag}" + tail = f" || {on_failure}" if on_failure else "" + return ( + f'grep -qxF {shlex.quote(mark)} "$PROFILE" 2>/dev/null || ' + f"printf '%s\\n' {shlex.quote(mark)} {shlex.quote(line)} >> \"$PROFILE\"{tail}" + ) + + def _install_line(tool: Tool) -> str: args = " ".join(shlex.quote(arg) for arg in tool.install_args) return ( @@ -205,11 +246,15 @@ def provision_script(tools: Sequence[Tool] = REQUIRED_TOOLS) -> str: # (since the check above is `command -v`) reinstalled from scratch # on every single launch. _PROFILE_RESOLUTION, - 'grep -q "\\.pixi/bin" "$PROFILE" 2>/dev/null || ' - 'echo \'export PATH="$HOME/.pixi/bin:$PATH"\' >> "$PROFILE" || failed=1', - 'grep -q "pixi/envs/claude-shim" "$PROFILE" 2>/dev/null || ' - 'echo \'[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && ' - 'export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"\' >> "$PROFILE" || failed=1', + _profile_prepend( + "pixi-bin", 'export PATH="$HOME/.pixi/bin:$PATH"', on_failure="failed=1" + ), + _profile_prepend( + "claude-shim-bin", + '[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && ' + 'export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"', + on_failure="failed=1", + ), ] ) return "\n".join( @@ -454,11 +499,15 @@ def transfer_script(payload: HostPayload) -> str: """ version = payload.claude_version claude_rel = f"{CLAUDE_VERSIONS_RELPATH}/{version}" + # Prepended, and appended *last*, because winning is the whole point: the + # container this lend exists for already has a shim earlier on PATH, put + # there by a line further up this same file. The lent binary only becomes + # the `claude` a session finds -- and the next probe only reads + # `provisioned` -- because ~/.local/bin goes in front of it. profile_lines = "\n".join( [ _PROFILE_RESOLUTION, - 'grep -q "\\.local/bin" "$PROFILE" 2>/dev/null || ' - 'echo \'export PATH="$HOME/.local/bin:$PATH"\' >> "$PROFILE"', + _profile_prepend("local-bin", 'export PATH="$HOME/.local/bin:$PATH"'), ] ) return "\n".join( @@ -613,11 +662,16 @@ def ensure_tools(workspace: str, runner) -> bool: # # Accepted residual: a container that keeps rejecting the lend -- # a different libc or architecture -- while carrying a shim - # re-attempts a ~5s failing transfer on every `devpod up`. Breaking - # that loop needs per-container retry state persisted somewhere, - # which is more machinery than the case is worth, and ensure_tools - # never runs on the fast-attach path: this only ever rides an `up` - # that already took seconds to minutes. + # re-attempts one failing transfer on every `devpod up`, paying for + # the tar and the stream before the arch/libc gate refuses the + # binaries. What that costs end to end has not been measured; the + # figure this comment used to carry was inherited, not taken. The + # one part of it that has been measured is staging the tar (#158: + # ~0.13s warm, ~2.6s cold), which is a fraction of it. Breaking the + # loop needs per-container retry state persisted somewhere, which is + # more machinery than the case is worth, and ensure_tools never runs + # on the fast-attach path: this only ever rides an `up` that already + # took seconds to minutes. return True # ProbeResult.ABSENT: the cold flow, with a tool genuinely missing. diff --git a/test/unit/test_tools.py b/test/unit/test_tools.py index 6e8d79b3..49152bef 100644 --- a/test/unit/test_tools.py +++ b/test/unit/test_tools.py @@ -168,6 +168,60 @@ def test_a_custom_tool_set_is_honoured(self): assert "command -v gh" not in script +class TestProfileGuards: + """Every "have I already edited this profile?" question, in both scripts. + + Asked once here rather than per script, because the property is the same + wherever it is asked and the answer decides whether a workspace ever stops + doing work it has already done. + """ + + @staticmethod + def _guards(script): + """The guard half of every line that appends to the login profile.""" + guards = [ + line.partition("||")[0] for line in script.splitlines() if '>> "$PROFILE"' in line + ] + assert guards, "a script that never edits the profile has nothing to guard" + return guards + + @pytest.mark.parametrize("name", ["provision", "transfer"]) + def test_a_guard_asks_about_devlaunchs_own_line_not_about_a_directory(self, name, tmp_path): + """The guard means "have *we* already prepended our directory here", and + the only evidence of that which devlaunch owns is the mark it writes. + + Asking instead whether the directory is mentioned anywhere in the file + makes the answer a base image's to give. Ubuntu's stock ~/.profile + prepends ~/.local/bin itself -- so on this repo's own base image that + question answered "already done" about work nobody had done, the lent + binary never reached the front of PATH, and every launch re-paid the + transfer. A directory name may appear in what is appended; it may not + appear in what decides whether to append. + """ + script = ( + provision_script() + if name == "provision" + else tools.transfer_script(fake_payload(tmp_path)) + ) + for guard in self._guards(script): + assert tools.PROFILE_MARK in guard + for owned_by_the_image in (".local/bin", ".pixi/bin", "pixi/envs/claude-shim"): + assert owned_by_the_image not in guard + + @pytest.mark.parametrize("name", ["provision", "transfer"]) + def test_a_guard_matches_a_whole_line_not_a_fragment_of_one(self, name, tmp_path): + """A mark is only devlaunch's if nothing longer can pass for it: an + exact-line, fixed-string match, so neither a regex metacharacter nor a + longer line that happens to contain the mark counts as a hit.""" + script = ( + provision_script() + if name == "provision" + else tools.transfer_script(fake_payload(tmp_path)) + ) + for guard in self._guards(script): + assert guard.startswith("grep -qxF ") + + def fake_payload(tmp_path) -> tools.HostPayload: """A payload made of scratch files, so no test ships a real 300MB binary.""" claude = tmp_path / "claude-2.0.1" @@ -199,6 +253,52 @@ def fake_payload(tmp_path) -> tools.HostPayload: "devlaunch-probe claude /ws/.pixi/envs/claude-shim/bin/claude\n" ) +# ~/.profile exactly as mcr.microsoft.com/devcontainers/base:ubuntu-24.04 ships +# it -- the base image .devcontainer/Dockerfile builds on -- copied out of the +# pulled image verbatim. The load-bearing part is the last block: Ubuntu's +# default profile puts ~/.local/bin on PATH itself, near the top of the file, +# long before anything devlaunch or the devcontainer feature appends. A guard +# that looks for the *directory* rather than for its own line reads that block +# as its own work and skips an append the workspace needs. +UBUNTU_STOCK_PROFILE = """\ +# ~/.profile: executed by the command interpreter for login shells. +# This file is not read by bash(1), if ~/.bash_profile or ~/.bash_login +# exists. +# see /usr/share/doc/bash/examples/startup-files for examples. +# the files are located in the bash-doc package. + +# the default umask is set in /etc/profile; for setting the umask +# for ssh logins, install and configure the libpam-umask package. +#umask 022 + +# if running bash +if [ -n "$BASH_VERSION" ]; then + # include .bashrc if it exists + if [ -f "$HOME/.bashrc" ]; then +\t. "$HOME/.bashrc" + fi +fi + +# set PATH so it includes user's private bin if it exists +if [ -d "$HOME/bin" ] ; then + PATH="$HOME/bin:$PATH" +fi + +# set PATH so it includes user's private bin if it exists +if [ -d "$HOME/.local/bin" ] ; then + PATH="$HOME/.local/bin:$PATH" +fi +""" + +# What .devcontainer/Dockerfile appends to that same file, verbatim, and in +# this order. The shim directory is prepended *last*, so it wins over +# everything above it -- including Ubuntu's own ~/.local/bin block. +DEVCONTAINER_PROFILE_LINES = """\ +export PATH="$HOME/.pixi/bin:$PATH" +# Workaround: pixi trampoline fails for bash scripts, so add env bin directly +[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH" +""" + class TestProbeScript: """The probe runs in someone else's container and decides whether to lend. @@ -223,11 +323,13 @@ def _bare_path(tmp_path): external the probe uses is linked in by hand. """ sysbin = tmp_path / "sysbin" - sysbin.mkdir() + sysbin.mkdir(exist_ok=True) for command in ("readlink",): found = shutil.which(command) assert found, f"the test host needs {command}" - (sysbin / command).symlink_to(found) + link = sysbin / command + if not link.exists(): + link.symlink_to(found) return [sysbin] _KEEP_HOME = object() @@ -261,6 +363,37 @@ def _answer(self, tmp_path, home, path_dirs=(), *, home_env=_KEEP_HOME): assert result.returncode == 0, result.stderr return tools.ProbeResult.parse(result.stdout) + def _answer_after_login(self, tmp_path, home): + """The same answer, but with PATH built by the home's own login profile. + + `_probe` runs the script under `bash -lc`, so in a real container every + directory the probe searches was put there by the profile -- the base + image's block, the devcontainer's appended lines and the transfer's + prepend, in whatever order they ended up in the file. Handing the probe + a PATH instead (as the other cases here do, to keep them hermetic) + hides exactly that ordering, which is the thing a lend depends on. + + `$HOME/.profile` is sourced explicitly rather than using `bash -l`, + because `-l` would also source the *test host's* /etc/profile and drag + its system directories -- and whatever `claude` the developer has -- + into a run that is meant to see only this scratch home. The user + profile is the file under test and the only one either script writes. + """ + env = { + "HOME": str(home), + "PATH": ":".join(str(d) for d in self._bare_path(tmp_path)), + } + script = f'. "$HOME/.profile"\n{tools.probe_script()}' + result = subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", script], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + return tools.ProbeResult.parse(result.stdout) + @staticmethod def _official_claude(home, version="2.0.1"): """What the official installer leaves behind: one binary per version @@ -318,31 +451,31 @@ def test_a_real_claude_with_no_gh_is_absent_not_lendable(self, tmp_path): self._official_claude(home) assert self._answer(tmp_path, home, [home / ".local/bin"]) is tools.ProbeResult.ABSENT - def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): - """Convergence, run for real end to end: the transfer script unpacks - into a scratch $HOME, and the probe is then asked about that same home. + @staticmethod + def _lend(tmp_path, home, version="2.0.1"): + """Run the real transfer script against a scratch $HOME, for real. - The two scripts have to agree or the lending never terminates -- a lend - the next probe does not recognise means every `up` for the rest of that - workspace's life re-pays the transfer. + A host payload is built out of scratch binaries, tarred the way + `_transfer` tars it, and streamed into `transfer_script` on stdin -- + so what lands in `home`, and what the script writes into that home's + login profile, is what a real lend leaves behind. """ - home = self._home(tmp_path) - source = tmp_path / "host" - source.mkdir() + source = tmp_path / f"host-{version}" + source.mkdir(exist_ok=True) binaries = {} - for name in ("claude-2.0.1", "gh"): + for name in (f"claude-{version}", "gh"): binary = source / name binary.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") binary.chmod(0o755) binaries[name] = binary payload = tools.HostPayload( - claude_version="2.0.1", + claude_version=version, members=( - (binaries["claude-2.0.1"], ".local/share/claude/versions/2.0.1"), + (binaries[f"claude-{version}"], f".local/share/claude/versions/{version}"), (binaries["gh"], ".local/bin/gh"), ), ) - bundle = tmp_path / "tools.tar" + bundle = tmp_path / f"tools-{version}.tar" with tarfile.open(bundle, mode="w") as tar: for member_source, arcname in payload.members: tar.add(member_source, arcname=arcname) @@ -356,8 +489,53 @@ def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): check=False, ) assert lend.returncode == 0, lend.stderr + + def test_the_layout_a_lend_leaves_behind_answers_provisioned(self, tmp_path): + """Convergence, run for real end to end: the transfer script unpacks + into a scratch $HOME, and the probe is then asked about that same home. + + The two scripts have to agree or the lending never terminates -- a lend + the next probe does not recognise means every `up` for the rest of that + workspace's life re-pays the transfer. + """ + home = self._home(tmp_path) + self._lend(tmp_path, home) assert self._answer(tmp_path, home, [home / ".local/bin"]) is tools.ProbeResult.PROVISIONED + def test_a_lend_converges_on_the_image_this_repo_ships(self, tmp_path): + """Convergence where it actually has to hold: a shim container whose + login profile is Ubuntu's stock one plus the lines this repo's own + devcontainer appends, with PATH decided by *sourcing that profile* + rather than handed to the probe by the test. + + PATH order is the whole mechanism by which a lend takes effect -- the + lent binary only wins because the transfer prepends `~/.local/bin` to + the profile -- so a convergence test that builds PATH itself asserts a + world in which the profile edit cannot be wrong. This one lets the + profile decide, which is the only way the edit being skipped is + visible: skipped, the shim stays in front, every `up` re-pays the tar, + and the workspace never converges. + """ + home = self._home(tmp_path) + (home / ".profile").write_text( + UBUNTU_STOCK_PROFILE + DEVCONTAINER_PROFILE_LINES, encoding="utf-8" + ) + shim = home / ".pixi/envs/claude-shim/bin/claude" + shim.parent.mkdir(parents=True) + shim.write_text("#!/bin/sh\necho 'downloading 285MB' >&2\n", encoding="utf-8") + shim.chmod(0o755) + gh = home / ".pixi/bin/gh" + gh.parent.mkdir(parents=True) + gh.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + gh.chmod(0o755) + + assert self._answer_after_login(tmp_path, home) is tools.ProbeResult.LENDABLE + self._lend(tmp_path, home) + assert self._answer_after_login(tmp_path, home) is tools.ProbeResult.PROVISIONED + # And the second lend never happens: converged means converged, not + # converged-then-drifting-back. + assert self._answer_after_login(tmp_path, home) is tools.ProbeResult.PROVISIONED + @staticmethod def _nested_shim(home): """A downloader that parked itself *inside* the versions directory.