diff --git a/.devcontainer/claude-code/install.sh b/.devcontainer/claude-code/install.sh index 020018f3..b11189c7 100755 --- a/.devcontainer/claude-code/install.sh +++ b/.devcontainer/claude-code/install.sh @@ -81,23 +81,26 @@ install_claude_code() { pixi global install --channel https://prefix.dev/blooop claude-shim fi - # Add pixi bin path to user's profile if not already there - local profile="$TARGET_HOME/.profile" - local pixi_path_line='export PATH="$HOME/.pixi/bin:$PATH"' - if [ -f "$profile" ] && ! grep -q '\.pixi/bin' "$profile"; then - echo "$pixi_path_line" >> "$profile" - elif [ ! -f "$profile" ]; then - echo "$pixi_path_line" > "$profile" - chown "$TARGET_USER:$TARGET_USER" "$profile" 2>/dev/null || true - fi - - # Workaround: pixi trampoline fails for bash scripts, so add env bin directly - # This conditionally adds the path only if the env exists - local env_path_line='[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"' - if [ -f "$profile" ] && ! grep -q 'pixi/envs/claude-shim' "$profile"; then - echo "# Workaround: pixi trampoline fails for bash scripts" >> "$profile" - echo "$env_path_line" >> "$profile" - fi + # Put pixi's bin directory on the user's login PATH, and the claude-shim + # env's bin beside it (the pixi trampoline fails for packages that ship a + # shell script -- the same workaround devlaunch's provision script + # carries). Each line goes in under a `# devlaunch:` mark, and the guard + # is an exact-line match on that mark: this file is the *user's* profile, + # not this feature's, and a guard that asks about a directory name reads + # a base image's own PATH block as work already done -- the misread that + # cost devlaunch a re-paid transfer on every launch (#164). + # + # Both edits are devlaunch's _profile_prepend output, verbatim. The marks + # are content hashes of the lines they guard, which sh cannot rederive, + # so they are pasted rather than computed -- and matching them is what + # lets this installer (at image build) and devlaunch's provision script + # (at `up`) recognise each other's work instead of each appending its own + # copy of the same line. A test pins these fragments byte-for-byte; + # regenerate with devlaunch.tools._profile_prepend if a line changes. + PROFILE="$TARGET_HOME/.profile" + grep -qxF '# devlaunch: 6b593c3a6327' "$PROFILE" 2>/dev/null || printf '%s\n' '# devlaunch: 6b593c3a6327' 'export PATH="$HOME/.pixi/bin:$PATH"' >> "$PROFILE" + grep -qxF '# devlaunch: 12897b113bea' "$PROFILE" 2>/dev/null || printf '%s\n' '# devlaunch: 12897b113bea' '[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"' >> "$PROFILE" + chown "$TARGET_USER:$TARGET_USER" "$PROFILE" 2>/dev/null || true # Verify installation by checking the trampoline exists (don't run it - that triggers download) local pixi_bin_path="$TARGET_HOME/.pixi/bin" diff --git a/CHANGELOG.md b/CHANGELOG.md index 879c36c9..cf9929ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Two guards around the tools lend were weaker than what was claimed of them** + ([#166](https://github.com/blooop/devlaunch/issues/166)) — the follow-ups the + final re-verdict on [#164](https://github.com/blooop/devlaunch/pull/164) judged + non-blocking, worked once. "The probe never executes the candidate `claude`" + was asserted by scrubbing the rendered script's text for an invocation, and a + probe that executed `"$(command -v claude)"` walked straight past it — once the + lookup is scrubbed, no literal `claude` remains in the execution. The property + is now proven behaviourally: the probe runs against a claude that records + being invoked (shell builtins only, so the recorder cannot silently fail on + the probe's stripped `PATH`), and the record must stay empty. The shipped + probe passes — it never did execute the candidate — so this hardens a guard + rather than fixing a live defect; the text scrub stays as a complement under + a docstring that no longer overclaims. + + The `# devlaunch:` marks guarding profile edits used hand-picked tags, and + nothing kept the tags distinct: two lines under one mark silently drop + whichever is appended second, with every script still exiting 0. The tag is + now derived from the content of the line it guards, so a collision is + unrepresentable rather than asserted. A profile marked by an older devlaunch + gains one duplicate `PATH` entry the first time it is seen — the cost the + module already accepts for any mark change — and nothing after. + + `.devcontainer/claude-code/install.sh` now carries the same guards. It was + excluded from the #164 sweep on the stated grounds that it guards its own + line in its own file; that reason was wrong — `$TARGET_HOME/.profile` is the + *user's* login profile, and the installer's guards substring-matched + `.pixi/bin` and `pixi/envs/claude-shim`, the very strings that sweep stopped + matching. Latent rather than live (the stock base-image profile contains + neither string), and corrected along with the record. Its two edits are now + devlaunch's own rendered fragments verbatim, marks included, so the installer + (at image build) and the provision script (at `up`) recognise each other's + work instead of each appending its own copy of the same line. + - **`dl rm` could destroy unsaved work without being asked for `--force`** ([#174](https://github.com/blooop/devlaunch/issues/174)). The guard read the clone directory off `local_path` while the delete fell back to the derived path diff --git a/devlaunch/tools.py b/devlaunch/tools.py index 5843891a..31020549 100644 --- a/devlaunch/tools.py +++ b/devlaunch/tools.py @@ -55,6 +55,7 @@ """ import enum +import hashlib import json import logging import os @@ -182,16 +183,27 @@ 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: +def _profile_prepend(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 + The line is written under a `PROFILE_MARK` comment, 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. + The mark names the line by a hash of its content rather than by a + hand-picked tag, so two different lines cannot share a mark: under a + shared mark, whichever line is appended second is silently dropped -- + its guard finds the first line's mark and reads it as its own work + already done -- and every script involved still exits 0. Hand-picked + tags left that collision one edit away and guarded by nothing; + derivation makes it unrepresentable rather than asserted. Identical + lines sharing one mark is not a collision but the dedupe itself, and + twelve hex characters of SHA-256 cannot collide across the handful of + lines devlaunch will ever append. + 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. @@ -199,9 +211,10 @@ def _profile_prepend(tag: str, line: str, on_failure: str = "") -> str: 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. + price of the guard no longer being able to lie. Renaming the marks -- + which switching them to hashes did once -- costs exactly that. """ - mark = f"{PROFILE_MARK} {tag}" + mark = f"{PROFILE_MARK} {hashlib.sha256(line.encode('utf-8')).hexdigest()[:12]}" tail = f" || {on_failure}" if on_failure else "" return ( f'grep -qxF {shlex.quote(mark)} "$PROFILE" 2>/dev/null || ' @@ -246,11 +259,8 @@ 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, + _profile_prepend('export PATH="$HOME/.pixi/bin:$PATH"', on_failure="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", @@ -507,7 +517,7 @@ def transfer_script(payload: HostPayload) -> str: profile_lines = "\n".join( [ _PROFILE_RESOLUTION, - _profile_prepend("local-bin", 'export PATH="$HOME/.local/bin:$PATH"'), + _profile_prepend('export PATH="$HOME/.local/bin:$PATH"'), ] ) return "\n".join( diff --git a/test/unit/test_tools.py b/test/unit/test_tools.py index 49152bef..410ae7af 100644 --- a/test/unit/test_tools.py +++ b/test/unit/test_tools.py @@ -2,6 +2,7 @@ import io import os +import pathlib import shlex import shutil import subprocess @@ -134,13 +135,6 @@ def test_it_writes_to_the_profile_bash_actually_reads(self): assert script.index(".bash_profile") < script.index(".bash_login") assert script.index(".bash_login") < script.index('PROFILE="$HOME/.profile"') - def test_the_profile_is_not_appended_to_twice(self): - """Every profile edit is guarded, so relaunching cannot grow the file.""" - script = provision_script() - profile_writes = [line for line in script.splitlines() if '>> "$PROFILE"' in line] - assert profile_writes - assert all(line.startswith("grep -q") for line in profile_writes) - def test_a_profile_that_cannot_be_written_is_a_failure(self): """Installed but not on PATH is not the guarantee this module makes.""" script = provision_script() @@ -168,24 +162,47 @@ def test_a_custom_tool_set_is_honoured(self): assert "command -v gh" not in script +# The devcontainer feature's installer edits the same file the scripts above +# do -- `$TARGET_HOME/.profile` is the *user's* login profile, not a file the +# feature owns -- so its guards are held to the same properties. It was +# excluded from the #164 sweep on the stated grounds that it guards its own +# line in its own file; that reason was wrong, which is why it is covered here +# rather than trusted. +FEATURE_INSTALLER = ( + pathlib.Path(__file__).resolve().parents[2] / ".devcontainer/claude-code/install.sh" +) + + class TestProfileGuards: - """Every "have I already edited this profile?" question, in both scripts. + """Every "have I already edited this profile?" question, in every script. 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. """ + SCRIPTS = ["provision", "transfer", "feature"] + + @staticmethod + def _script(name, tmp_path): + if name == "provision": + return provision_script() + if name == "transfer": + return tools.transfer_script(fake_payload(tmp_path)) + return FEATURE_INSTALLER.read_text(encoding="utf-8") + @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 + line.strip().partition("||")[0] + for line in script.splitlines() + if '>> "$PROFILE"' in line or '>> "$profile"' in line ] assert guards, "a script that never edits the profile has nothing to guard" return guards - @pytest.mark.parametrize("name", ["provision", "transfer"]) + @pytest.mark.parametrize("name", SCRIPTS) 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. @@ -198,29 +215,77 @@ def test_a_guard_asks_about_devlaunchs_own_line_not_about_a_directory(self, name 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)) - ) + script = self._script(name, 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_every_line_lands_exactly_once_however_often_the_scripts_run(self, tmp_path): + """Two different lines may never share a mark, and a rerun may never + append again. Both are the same property -- the mark is what decides + "already done" -- and both failure modes are silent: two lines under + one mark drop whichever comes second (its guard finds the first + line's mark and reads it as its own work), and a missed dedupe grows + the profile on every launch. Nothing exits non-zero either way. + + Run for real rather than compared as text, and across *both* scripts + into one profile, because that is where the lines meet: a provision + and a lend edit the same file over a workspace's life, so a + collision between their marks is just as fatal as one within either. + """ + profile = tmp_path / "profile" + appends = [] + for name in self.SCRIPTS: + script = self._script(name, tmp_path) + appends.extend(line.strip() for line in script.splitlines() if '>> "$PROFILE"' in line) + assert len(appends) == 5, "a new PATH line belongs in this test's expectations" + for _ in range(2): + subprocess.run( + [shutil.which("bash") or "/bin/bash", "-c", "\n".join(appends)], + env={**os.environ, "PROFILE": str(profile)}, + check=True, + ) + written = profile.read_text(encoding="utf-8").splitlines() + for line in ( + 'export PATH="$HOME/.pixi/bin:$PATH"', + '[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && ' + 'export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"', + 'export PATH="$HOME/.local/bin:$PATH"', + ): + assert written.count(line) == 1, f"{line!r} appended {written.count(line)} times" + + @pytest.mark.parametrize("name", SCRIPTS) 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): + for guard in self._guards(self._script(name, tmp_path)): assert guard.startswith("grep -qxF ") + def test_the_feature_installer_writes_the_very_fragments_devlaunch_renders(self): + """The installer's two profile edits are `_profile_prepend`'s own + output, verbatim -- same derived mark, same guard, same line. + + Pinned byte-for-byte rather than by shape, because the marks are + content hashes a shell script cannot derive for itself: hardcoded, + they drift the moment either side's line changes, and a drifted mark + quietly costs a workspace one duplicate PATH entry per line -- the + installer runs at image build, the provision script at `up`, and only + matching marks let the second writer recognise the first's work. + """ + installer = FEATURE_INSTALLER.read_text(encoding="utf-8") + for line in ( + 'export PATH="$HOME/.pixi/bin:$PATH"', + '[ -d "$HOME/.pixi/envs/claude-shim/bin" ] && ' + 'export PATH="$HOME/.pixi/envs/claude-shim/bin:$PATH"', + ): + fragment = tools._profile_prepend(line) # pylint: disable=protected-access + assert fragment in installer, ( + f"the feature installer no longer carries the rendered append for {line!r}; " + "regenerate it with tools._profile_prepend" + ) + def fake_payload(tmp_path) -> tools.HostPayload: """A payload made of scratch files, so no test ships a real 300MB binary.""" @@ -624,11 +689,39 @@ def test_it_resolves_the_link_rather_than_trusting_the_path_entry(self): 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: + def test_it_never_runs_the_candidate_claude(self, tmp_path): + """Shim-proofness, asked of a run rather than of the script's text: *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. + detect, so the probe runs here against a claude that records being + invoked, and the record must stay empty. + + The recorder is shell builtins only -- `:` and a redirection -- because + the probe runs on a stripped PATH carrying nothing but `readlink`. A + marker that needed an external binary (`touch`, `date`) would fail to + record the very invocation it exists to catch, and this test would + pass for the wrong reason. + """ + home = self._home(tmp_path) + shim = home / ".local/bin/claude" + shim.parent.mkdir(parents=True, exist_ok=True) + shim.write_text('#!/bin/sh\n: > "$HOME/shim-was-executed"\n', encoding="utf-8") + shim.chmod(0o755) + answer = self._answer(tmp_path, home, [self._gh(home)]) + # The answer proves the run reached the end of the script: a probe + # that crashed before resolving the claude would leave the record + # empty too, and this test would be guarding a script that never ran. + assert answer is tools.ProbeResult.LENDABLE + assert not (home / "shim-was-executed").exists() + + def test_the_script_text_confines_claude_to_the_known_lookups(self): + """A complement to the behavioural test above, not the guard itself. + + The run above proves that one real probe executed nothing; this scrub + confines where the name may appear at all, which is what catches an + invocation parked on a branch that run does not take. Alone it is + weaker than it reads -- an execution spelled `"$(command -v claude)"` + contains no literal `claude` once the lookup is scrubbed, and walks + straight past it -- which is why it no longer stands alone. """ script = tools.probe_script() scrubbed = ( @@ -977,12 +1070,6 @@ def test_progress_goes_to_stderr_not_stdout(self, tmp_path): assert "exec >&2" in script assert script.index("exec >&2") < script.index("echo") - def test_the_profile_edit_is_guarded_like_the_network_installs(self, tmp_path): - script = self._script(tmp_path) - profile_writes = [line for line in script.splitlines() if '>> "$PROFILE"' in line] - assert profile_writes - assert all(line.startswith("grep -q") for line in profile_writes) - def test_the_stream_the_container_receives_is_that_tar(self, tmp_path): """End to end: what `tar xf -` reads on the other side has to be a readable archive of exactly the lent files, under the arcnames the