diff --git a/openRepoTools b/openRepoTools index 3cb5293..69f9f32 100755 --- a/openRepoTools +++ b/openRepoTools @@ -71,6 +71,13 @@ SELF="${BASH_SOURCE[0]:-$0}" MODE="" WORKDIR="" STAGED="" +# THE PREFIX OF THE TEMPORARY A PLACEMENT IS MAKING (#167), named here for the +# `EXIT` trap `workdir` sets, which removes `"$PLACING"*`: `place_by_rename` +# removes its temporary on every failure it sees, and this is what removes it +# on the ones it cannot see — a signal, or an `errexit`. A PREFIX, set before +# `mktemp` runs, so no instant exists in which the file is there and the trap +# does not know its name (Copilot round 2 on #172). +PLACING="" say() { printf '%s\n' "$*"; } die() { printf '\nREFUSED: %s\n' "$1" >&2; exit "${2:-2}"; } @@ -217,7 +224,7 @@ USAGE workdir() { if [ -z "$WORKDIR" ]; then WORKDIR="$(mktemp -d)" - trap 'rm -rf -- "$WORKDIR"' EXIT + trap 'rm -rf -- "$WORKDIR"; [ -z "$PLACING" ] || rm -f -- "$PLACING"*' EXIT fi } @@ -979,6 +986,107 @@ mode_is() { # [ -n "$(find "$2" -maxdepth 0 -perm "$1" -print 2>/dev/null)" ] } +# WHO OWNS A PATH, as `:`, and nothing where it cannot be read — +# asked without `stat` for the reason `mode_is` gives. `ls -n` prints both as +# numbers, in its third and fourth fields, on GNU, BSD and busybox alike, and +# `-d` keeps the answer about the path itself. The `|| :` keeps a path `ls` +# cannot read an empty answer rather than a `pipefail` exit. +owner_of() { # + { ls -lnd -- "$1" 2>/dev/null || :; } | awk 'NR == 1 { print $3 ":" $4 }' +} + +# EVERY FILE `--install` PLACES IS WRITTEN BESIDE ITS TARGET AND RENAMED OVER IT +# (opensoft/openRepoTools#167): a temporary in the target's own directory, the +# bytes, the mode, then `mv -f`. It was `cp` onto the target, which keeps the +# target's INODE and rewrites its bytes — and bash reads a script from its file +# while it runs it, at an offset, so a lane tool that was running when an +# install replaced it went on reading the NEW text from the OLD offset. Measured +# on 2026-09-30: `A.sh: line 4: ript,: command not found`, then the new text's +# commands run from its middle. `lanes-edit.sh`, `lane-start` and the landers +# run for minutes, so a repair install at a busy moment could corrupt a lane's +# register write mid-flight. A rename puts a NEW inode at the path in one step: +# a process that has the old one open reads the old text to its end, and the +# next one to start reads the new. The SAME directory, because a rename is +# atomic only within one filesystem, and the same shape the receipt and +# `settings.json` were already written in. +# +# THE MODE GOES ON THE TEMPORARY, so the target never exists at the wrong mode +# for even an instant, and a stamp that fails (#48) leaves whatever was at the +# target exactly as it was. +# +# EVERY FAILURE REMOVES THE TEMPORARY, then refuses naming the target, and a +# run killed while one exists leaves nothing behind either: `$PLACING` holds +# its PREFIX — `..openrepotools..` — from BEFORE `mktemp` runs +# until after the `mv`, and the `EXIT` trap removes whatever starts with it. A +# name the trap learnt only from `mktemp`'s answer had a window, between the +# file being made and the answer being assigned, in which a signal left it +# behind (Copilot round 2 on #172); the pid keeps the prefix this run's own. +# The leading dot keeps it out of a listing, and the suffix after the name +# keeps it out of every glob a reader of these directories takes (`*.md` in +# `commands/`). +# +# THE OWNER STAYS WHO IT WAS (Copilot on #172). A rename puts the TEMPORARY's +# inode at the path, and the temporary belongs to whoever runs this — so a root +# `--install` into another user's home, which this file goes out of its way to +# allow (search "WHAT `-w` DOES NOT PROVE"), would hand that user root-owned +# copies that their own next `--install` refuses as unwritable. `cp` kept the +# owner by keeping the inode; this keeps it by giving the temporary the +# target's uid and gid before the rename. Where that `chown` is refused and the +# OWNER differs — an account placing over a file another account owns, which +# only root may give away — the placement refuses rather than take the file +# over. Where only the GROUP differs and cannot be kept, it is placed anyway: +# at 755 and 644 the group's bits are the other bits, so nobody gains or loses +# a permission by it (Copilot round 2 on #172 asked that it not be silent, and +# this is why it may be). And an owner NOBODY COULD READ is not "nothing to +# keep": `owner_of` answers nothing where `ls` cannot say, which is also what an +# absent target gives, so a target that EXISTS with no readable owner is +# refused before the rename rather than handed this run's owner (Copilot on +# #172 at bef4b85). +# +# A DIRECTORY AT THE TARGET IS REFUSED HERE as well as in planning, because `mv` +# moves a file INTO a directory named as its destination rather than failing, +# and the planners are a phase earlier. Two installs that race (#106) each +# still place whole files; serializing them is #106's lock, not this. +place_by_rename() { # + local source="$1" target="$2" mode="$3" rest="$4" tmp="" was="" now="" + PLACING="$(dirname -- "$target")/.${target##*/}.openrepotools.$$." + tmp="$(mktemp -- "${PLACING}XXXXXX")" || + { rm -f -- "$PLACING"*; PLACING=""; die "could not create a temporary file beside $target, so it was not placed. + $rest"; } + cp -- "$source" "$tmp" || + { rm -f -- "$tmp"; PLACING=""; die "could not write $target, so it was not placed and what was there is untouched. + $rest"; } + chmod "$mode" "$tmp" || + { rm -f -- "$tmp"; PLACING=""; die "could not set mode $mode on $target, so it was not placed and what was there is untouched. + $rest"; } + was="$(owner_of "$target")" + now="$(owner_of "$tmp")" + if [ -z "$was" ] && { [ -e "$target" ] || [ -L "$target" ]; }; then + rm -f -- "$tmp" + PLACING="" + die "could not read who owns $target, so it was not placed and what was there is untouched: a rename would give it this run's owner. + $rest" + fi + if [ -n "$was" ] && [ "$was" != "$now" ] && + ! chown -- "$was" "$tmp" 2>/dev/null && + [ "${was%%:*}" != "${now%%:*}" ]; then + rm -f -- "$tmp" + PLACING="" + die "$target belongs to uid ${was%%:*}, and this run cannot give the new copy that owner, so it was not placed and what was there is untouched. + $rest" + fi + if [ -d "$target" ]; then + rm -f -- "$tmp" + PLACING="" + die "$target is a directory, so nothing was placed there: \`mv\` would have put the file inside it. + $rest" + fi + mv -f -- "$tmp" "$target" || + { rm -f -- "$tmp"; PLACING=""; die "could not move the new $target into place, so what was there is untouched. + $rest"; } + PLACING="" +} + install_commands() { local dir="${OPENREPOTOOLS_BIN_DIR:-$HOME/.local/bin}" local name target source verb placed=0 @@ -1022,30 +1130,36 @@ install_commands() { else verb="already installed at $target (mode restored to 755)" fi - else - if [ -e "$target" ]; then verb="updated at $target"; else verb="installed at $target"; fi - cp -- "$source" "$target" - fi - # STAMPED EVERY TIME, AND ITS FAILURE IS A REFUSAL (#48). A copy that is - # not executable is not a command, and this was the ONE mode stamp in - # this file with no `die` beside it: under `set -e` a failure here left - # the shell's own 1, which this toolset spends on "findings were - # printed", as the answer from a run that was refusing with the commands - # before it already placed. - # - # AND NO OWNERSHIP PREFLIGHT IN FRONT OF IT, which is the decision - # `unplaceable_kind` already carries — search "WHAT `-w` DOES NOT PROVE" - # for the whole of it. `[ -O ]` is the question that would catch this - # first, and it is a guard no test in this suite can reach and one that - # would refuse a root `--install` into another user's home, where the - # stamp does in fact succeed. R-A9-15 is the ruling that a guard nobody - # can test is a guard nobody can rely on, so the stamp's own failure is - # the refusal, naming the file it could not mode. - chmod 755 "$target" || - die "could not set mode 755 on $target, so that command is not executable. + # STAMPED EVERY TIME, AND ITS FAILURE IS A REFUSAL (#48). A copy that + # is not executable is not a command, and this was the ONE mode stamp + # in this file with no `die` beside it: under `set -e` a failure here + # left the shell's own 1, which this toolset spends on "findings were + # printed", as the answer from a run that was refusing with the + # commands before it already placed. On THIS arm it is stamped in + # place — a mode is metadata, and no running copy reads it — and on + # the other `place_by_rename` stamps the temporary (#167). + # + # AND NO OWNERSHIP PREFLIGHT IN FRONT OF IT, which is the decision + # `unplaceable_kind` already carries — search "WHAT `-w` DOES NOT PROVE" + # for the whole of it. `[ -O ]` is the question that would catch this + # first, and it is a guard no test in this suite can reach and one that + # would refuse a root `--install` into another user's home, where the + # stamp does in fact succeed. R-A9-15 is the ruling that a guard nobody + # can test is a guard nobody can rely on, so the stamp's own failure is + # the refusal, naming the file it could not mode. + chmod 755 "$target" || + die "could not set mode 755 on $target, so that command is not executable. Whatever the lines above report is placed; this one has its bytes and not its mode, and the skills, the command files and the hook entries are not placed at all." + else + if [ -e "$target" ]; then verb="updated at $target"; else verb="installed at $target"; fi + # WRITTEN BESIDE ITSELF AND RENAMED OVER ITSELF, never rewritten in + # place (#167): a lane tool running from this path finishes on the + # text it started with. `place_by_rename` says why. + place_by_rename "$source" "$target" 755 "Whatever the lines above report is placed; the skills, the command files and the + hook entries are not placed at all." + fi placed=$((placed + 1)) say "$name: $verb" done @@ -1331,6 +1445,14 @@ plan_skill_targets() { # not this user's to write (#40, finding 4). The eleven were already placed # by the time `cp` met it, on the one path that promises all or none, so the # `-w` is asked here beside the `-L`. + # + # BOTH QUESTIONS OUTLIVE THE `cp` (#167). The write is a rename now, which + # follows no link and needs the DIRECTORY's write bit rather than the + # file's — so neither target would FAIL any more; each would be REPLACED, + # without a word. A link is somebody's link, and swapping it for a regular + # file unasked is the same act as writing through it, in the other + # direction (the receipt's own rule, `receipt_replace`); a read-only file + # is somebody saying "not this one". The refusal is the same refusal. for name in "${SKILLS[@]}"; do for target in "$(skills_home)/shared/skills/$name/SKILL.md" "$(claude_home)/skills/$name/SKILL.md"; do kind="$(unplaceable_kind "$target")" || continue @@ -1350,12 +1472,11 @@ plan_skill_targets() { [ -z "$bad" ] || die "\`--install\` places a REGULAR FILE IT CAN WRITE at each of these skill and command targets, and these are not that: $bad - \`cp\` follows a symlink, so installing over one would leave the skill - UNINSTALLED — the target stays a link — and would write these bytes into - whatever is on the other end of it; and a target this user cannot write is - a \`cp\` that fails with the eleven commands already placed, which is the - half-install this phase exists to prevent. NOTHING was installed and - nothing already installed was replaced. + Each file is placed by renaming a new copy over its path, so installing + over a symlink would replace somebody's link with a regular file, over a + directory would put the file inside it, and over a file this user cannot + write would replace a file somebody made read-only — none of it asked for. + NOTHING was installed and nothing already installed was replaced. Clear them, then re-run — \`rm -f\` takes a read-only file, because what a removal needs is the DIRECTORY's write bit and not the file's: rm -f -- $paths" @@ -1364,8 +1485,8 @@ $bad # THE SIXTEEN TARGETS, PROVED PLACEABLE BEFORE ANY OF THE THIRTY IS PLACED # (A9 Addendum 4, R-A9-12, ruled on F5 of the #24 review). # -# `cp` FOLLOWS A SYMLINK. The loop below tests `[ -e "$target" ]`, compares -# with `cmp -s` and then copies, and not one of the three can tell a regular +# `cp` FOLLOWS A SYMLINK. The loop below tested `[ -e "$target" ]`, compared +# with `cmp -s` and then copied, and not one of the three can tell a regular # file from a link to one — so against the bin directory the pre-move # `link-estates` actually leaves on a workstation, `--install` printed # `lane-start: updated at …`, `9 of 9 placed`, exit 0, and BOTH claims were @@ -1379,6 +1500,12 @@ $bad # fails the estate install that follows — the exact failure clause (f)(3) is # written about. # +# THE REFUSAL OUTLIVES THE `cp` (#167). The loop places by RENAME now, which +# follows no link: it would swap the link itself for a regular file, without a +# word, which is the same act in the other direction — the person's link gone +# and nothing on screen saying so. `plan_skill_targets` says the same of the +# files it walks. +# # A HOST STILL CARRYING THOSE SYMLINKS IS THE NORMAL PRE-ACT-5 CASE, not an # exotic one: adoption act 6 clears them on each workstation before the first # `--install` that carries this, and act 5 strips `brett-wip` to data. So the @@ -1392,14 +1519,17 @@ $bad # path is absent or is a regular file. # # `-L` AS WELL AS `-e`, because a DANGLING symlink is the sharper case: `[ -e ]` -# is false on one, and `cp` through it creates the file at the far end — a write -# into a repository this command was never told about. -# -# IT IS ASKED APART FROM THE WRITE, because the writes are not one write. `cp` -# needs the target file's own `w`; the SessionStart merge REPLACES its file -# through `mktemp` + `mv -f` and needs the DIRECTORY's, so a `settings.json` -# this user cannot write is one `--install` may still merge into — and a single -# walk that asked `-w` of all three would refuse an install that works. +# is false on one, so it reads as ABSENT. `cp` through it created the file at the +# far end — a write into a repository this command was never told about — and +# the rename that replaced `cp` (#167) would swap the link for a file unasked. +# +# IT IS ASKED APART FROM THE `-w`, because the `-w` is not asked of every path. +# Every write here is a rename since #167, and a rename needs the DIRECTORY's +# write bit and never the file's: `unplaceable_kind` asks the file's own `-w` of +# the files this installer owns, as a person's "not this one", while the +# SessionStart merge — which has always replaced `settings.json` through +# `mktemp` + `mv -f` — merges into one this user cannot write. A single walk that +# asked `-w` of all three would refuse an install that works. path_kind() { # local t="$1" if [ -L "$t" ]; then @@ -1417,7 +1547,7 @@ path_kind() { # return 1 } -# AND WHETHER `cp` MAY WRITE OVER IT: `-w` as well as `-f` (#40, finding 4). +# AND WHETHER IT MAY BE WRITTEN OVER: `-w` as well as `-f` (#40, finding 4). # This answered "a regular file" with `return 1` and asked nothing else, so # every existing file counted as one `--install` may place over — and # `plan_skill_targets` let a READ-ONLY `SKILL.md` or command file through. @@ -1428,10 +1558,17 @@ path_kind() { # # either: `chmod 444` on a file a person did not want overwritten is what a # person does. # +# THAT LAST SENTENCE IS NOW THE WHOLE REASON (#167). The write is a rename, which +# needs the directory's write bit and not the file's, so a read-only target would +# no longer FAIL half way — it would be replaced without a word. The question +# stays, so that a person's `chmod 444` is still the refusal it was. +# # WHAT `-w` DOES NOT PROVE IS THAT THE MODE STAMP CAN SUCCEED, and that limit is # deliberate (#44 round 1, the suppressed comment at `openRepoTools:635`). A -# GROUP-WRITABLE file owned by another account takes the `cp` and refuses the -# `chmod`, and the question that would settle it — `[ -O ]`, this euid against +# GROUP-WRITABLE file owned by another account passes `-w` and refuses the +# `chmod` where the stamp is made IN PLACE — the arm whose bytes were already +# right; the arm that writes stamps its own temporary (#167) — and the question +# that would settle it — `[ -O ]`, this euid against # the file's owner — is one no test in this suite can put: `tmp_path` cannot # make a second account, and it would refuse a root `--install` into another # user's home, where the `chmod` does in fact succeed. A guard no test reaches @@ -1543,12 +1680,11 @@ plan_install_targets() { [ -z "$bad" ] || die "\`--install\` places a REGULAR FILE IT CAN WRITE at each of these, and these are not that: $bad - \`cp\` follows a symlink, so installing over one would leave the command - UNINSTALLED — the target stays a link — and would write these bytes into - whatever is on the other end of it; and a target this user cannot write is - a \`cp\` that fails with the artifacts before it already placed, which is the - half-install this phase exists to prevent. NOTHING was installed and - nothing already installed was replaced. + Each command is placed by renaming a new copy over its path, so installing + over a symlink would replace somebody's link with a regular file, over a + directory would put the file inside it, and over a file this user cannot + write would replace a file somebody made read-only — none of it asked for. + NOTHING was installed and nothing already installed was replaced. Clear them, then re-run — \`rm -f\` takes a read-only file, because what a removal needs is the DIRECTORY's write bit and not the file's: rm -f -- $paths @@ -1826,11 +1962,15 @@ place_skill_and_hook() { else verb="already installed at $target (mode restored to 644)" fi + chmod 644 "$target" || die "could not set the mode on $target." else if [ -e "$target" ]; then verb="updated at $target"; else verb="installed at $target"; fi - cp -- "$WORKDIR/skill-$name.md" "$target" || - die "could not write $target. The $placed commands above are placed; - the skills and the hook are not." + # BY RENAME, as the bin directory's files are (#167). A session reads + # a `SKILL.md` whole rather than running it, so nothing here executes + # garbage — but one way to place a file is one rule to read, and a + # reader that opens it mid-install still sees the old text or the + # new, never a truncated one. + place_by_rename "$WORKDIR/skill-$name.md" "$target" 644 "The $placed commands above are placed; the skills and the hook are not." fi # 644: a SKILL.md is a document a session READS, not a command. The # sixteen in the bin directory are stamped 755 for the reason that loop @@ -1840,7 +1980,6 @@ place_skill_and_hook() { # arguing why adoption act 4b must delete workBenches#63's 0644 # loop; the deletion is required either way, because two writers of # one path is the defect and the mode was only its symptom. - chmod 644 "$target" || die "could not set the mode on $target." say "$name: $verb" done done @@ -1872,13 +2011,11 @@ place_skill_and_hook() { else verb="already installed at $target (mode restored to 644)" fi + chmod 644 "$target" || die "could not set the mode on $target." else if [ -e "$target" ]; then verb="updated at $target"; else verb="installed at $target"; fi - cp -- "$WORKDIR/command-$name.md" "$target" || - die "could not write $target. The $placed commands above are placed; - the skills, the command files and the hook are not." + place_by_rename "$WORKDIR/command-$name.md" "$target" 644 "The $placed commands above are placed; the skills, the command files and the hook are not." fi - chmod 644 "$target" || die "could not set the mode on $target." say "/$name: $verb" done done diff --git a/tests/test_install_by_rename.py b/tests/test_install_by_rename.py new file mode 100644 index 0000000..af2a484 --- /dev/null +++ b/tests/test_install_by_rename.py @@ -0,0 +1,179 @@ +# SPDX-License-Identifier: Apache-2.0 +"""THE WRITE `--install` PLACES EVERY FILE WITH, ON EVERY PLATFORM CI HAS (#167). + +`openRepoTools --install` writes each file it places to a temporary beside the +target and renames it over the target, so a lane tool that is RUNNING from that +path when an install replaces it finishes on the text it started with. The +installer's own tests (`tests/test_openrepotools_command.py`) prove that end to +end, but they carry `WINDOWS_SKIP` — the whole installer needs `jq`, `make` and +shebang execution, and on Windows the way in is WSL2 — so they say nothing +about Git Bash. Git Bash is exactly where the rename's semantics differ most: +renaming over a file another process has open is the one step of this write +that Windows can refuse. + +So this module runs the installer's OWN `place_by_rename` — its text, cut out +of the script, never a copy written here — under each platform's bash: `bash` +on PATH on Linux and macOS, and on Windows the Git Bash that ships with the +`git` running this suite (skipped where there is none). The function needs +only `die`, `$PLACING`, the `owner_of` beside it and coreutils, which is why +the two can be run on their own. +""" + +from __future__ import annotations + +import os +import re +import shutil +import subprocess + +from pathlib import Path + +import pytest + +from conftest import REPO + +COMMAND = REPO / "openRepoTools" + + +def platform_bash() -> str | None: + """`bash` on PATH where the OS is POSIX, and GIT BASH on Windows. + + On Windows `shutil.which("bash")` can answer WSL's launcher in System32, + which is not the shell this is about, so Git Bash is found through the + `git` that runs this suite: `/bin/bash.exe`, reached from either + `git --exec-path` (`/mingw64/libexec/git-core`) or `git.exe` itself + (`/cmd/git.exe`). + """ + if os.name != "nt": + return shutil.which("bash") + git = shutil.which("git") + if git is None: + return None + roots = [Path(git).resolve().parents[1]] + exec_path = subprocess.run([git, "--exec-path"], capture_output=True, + text=True, check=False).stdout.strip() + if exec_path: + roots.insert(0, Path(exec_path).parents[2]) + for root in roots: + candidate = root / "bin" / "bash.exe" + if candidate.is_file(): + return str(candidate) + return None + + +BASH = platform_bash() + +pytestmark = pytest.mark.skipif( + BASH is None, reason="no bash here (on Windows: no Git Bash beside git)") + +#: The same running text as the installer's end-to-end test: it announces +#: itself, then waits — `sleep` forks, which is what makes bash seek back and +#: read on from its file — until `$GO` exists, and finishes. Bounded. +OLD_RUNNING = ("#!/usr/bin/env bash\n" + "printf 'old: started\\n'\n" + 'i=0; until [ -e "$GO" ] || [ "$i" -ge 1200 ]; do ' + 'sleep 0.05; i=$((i + 1)); done\n') +OLD_TAIL = "printf 'old: finished\\n'\nexit 0\n" + + +def posix(path: Path) -> str: + """A path every bash here reads: Git Bash takes `C:/…` with forward + slashes, and a backslash would be an escape to it.""" + return path.as_posix() + + +def installer_function(name: str) -> str: + """One function as the installer carries it, cut out of the script.""" + text = COMMAND.read_text(encoding="utf-8") + match = re.search(rf"^{re.escape(name)}\(\) \{{[^\n]*\n.*?^\}}\n", text, + re.MULTILINE | re.DOTALL) + assert match, f"`{name}() {{` … `}}` is no longer in openRepoTools" + return match.group(0) + + +def place(tmp_path: Path, source: Path, target: Path, + mode: str = "755") -> subprocess.CompletedProcess: + """One placement, through the installer's function, under this bash.""" + harness = tmp_path / "harness.sh" + harness.write_bytes(( + "set -euo pipefail\n" + 'PLACING=""\n' + "die() { printf 'REFUSED: %s\\n' \"$1\" >&2; exit 2; }\n" + + installer_function("owner_of") + + installer_function("place_by_rename") + + 'place_by_rename "$1" "$2" "$3" "nothing else was placed."\n' + ).encode("utf-8")) + return subprocess.run([BASH, posix(harness), posix(source), posix(target), + mode], capture_output=True, text=True, check=False) + + +def temporaries(directory: Path) -> list[str]: + return sorted(p.name for p in directory.iterdir() + if ".openrepotools." in p.name) + + +def test_a_running_script_finishes_on_its_own_text_when_it_is_placed_over( + tmp_path): + """THE DEFECT #167 IS ABOUT, ON THIS PLATFORM'S BASH. + + A script is running from the target when the placement replaces it. The + running process must finish on its own text, and the path must hold the + new bytes afterwards. On Windows this is the question whether Git Bash's + `mv -f` may replace a file a running bash has open; a rewrite in place + would make the process read the new text from its old offset instead. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + target = bin_dir / "lanes-edit.sh" + target.write_bytes((OLD_RUNNING + OLD_TAIL).encode("utf-8")) + # A line of the NEW text starts at exactly the byte the old one resumes + # reading at, so a rewrite in place reads it, says so, and exits 7. + head = "#!/usr/bin/env bash\n# " + new = (head + "x" * (len(OLD_RUNNING) - len(head) - 1) + "\n" + + "printf 'new: read from the old offset\\n'; exit 7\n" + ).encode("utf-8") + source = tmp_path / "staged" + source.write_bytes(new) + + go = tmp_path / "go" + running = subprocess.Popen([BASH, posix(target)], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, + env={**os.environ, "GO": posix(go)}) + try: + assert running.stdout.readline() == "old: started\n" + placed = place(tmp_path, source, target) + assert placed.returncode == 0, placed.stdout + placed.stderr + go.touch() + out, err = running.communicate(timeout=120) + finally: + if running.poll() is None: + running.kill() + running.communicate() + assert (out, err, running.returncode) == ("old: finished\n", "", 0), ( + f"the running copy did not finish on its own text:\nstdout {out!r}\n" + f"stderr {err!r}\nexit {running.returncode}") + assert target.read_bytes() == new + if os.name != "nt": + assert target.stat().st_mode & 0o777 == 0o755 + assert temporaries(bin_dir) == [] + + +def test_a_directory_at_the_target_is_refused_and_leaves_no_temporary( + tmp_path): + """`mv` MOVES A FILE INTO A DIRECTORY NAMED AS ITS DESTINATION rather than + failing, so the function refuses one itself — the planners catch it a + phase earlier, and this is the step that must not depend on that. The + directory keeps exactly what it had, and the temporary is removed.""" + bin_dir = tmp_path / "bin" + target = bin_dir / "park" + target.mkdir(parents=True) + (target / "kept").write_bytes(b"kept\n") + source = tmp_path / "staged" + source.write_bytes(b"#!/usr/bin/env bash\n") + + placed = place(tmp_path, source, target) + assert placed.returncode == 2, placed.stdout + placed.stderr + assert "REFUSED:" in placed.stderr and "is a directory" in placed.stderr, ( + placed.stderr) + assert sorted(p.name for p in target.iterdir()) == ["kept"] + assert temporaries(bin_dir) == [] diff --git a/tests/test_openrepotools_command.py b/tests/test_openrepotools_command.py index 640c6c3..8feb7cf 100644 --- a/tests/test_openrepotools_command.py +++ b/tests/test_openrepotools_command.py @@ -28,8 +28,10 @@ import os import re import shutil +import signal import stat import subprocess +import time from datetime import datetime from pathlib import Path @@ -443,6 +445,488 @@ def test_install_replaces_a_copy_that_has_drifted(tmp_path, name): assert f"{other}: already installed at" in result.stdout, other +# --- every placement is a RENAME, never a rewrite in place (#167) ---------- +# +# `cp` onto an installed file keeps its inode and rewrites its bytes, and bash +# reads a script from its file WHILE it runs it: after every command that forks +# it seeks back to where it had got to and reads on from there. So a lane tool +# that was running when an install replaced it went on reading the NEW text +# from the OLD offset — measured on 2026-09-30 as `A.sh: line 4: ript,: command +# not found`, then the new text's commands run from its middle. A rename puts a +# new inode at the path and leaves the old one to whoever has it open. + +#: THE TEXT A LANE TOOL IS RUNNING WHEN THE INSTALL REPLACES IT. It announces +#: itself, then waits — `sleep` forks, which is what makes bash seek back and +#: read on from its file — until `$GO` exists, then finishes. Bounded, so a +#: test that fails before it says go leaves no process behind for long. +OLD_RUNNING = ("#!/usr/bin/env bash\n" + "printf 'old: started\\n'\n" + 'i=0; until [ -e "$GO" ] || [ "$i" -ge 1200 ]; do ' + 'sleep 0.05; i=$((i + 1)); done\n') +OLD_TAIL = "printf 'old: finished\\n'\nexit 0\n" + + +def new_text_read_from(offset: int) -> str: + """NEW bytes with a line of their own starting at exactly `offset`. + + That is the byte the running OLD text resumes reading at once its wait is + over. In place, the process reads THIS line instead of its own next one, + says so and exits 7. By rename it never sees these bytes at all. Built to + the offset rather than left to chance, so the failure a regression prints + is one line that says what happened. + """ + head = "#!/usr/bin/env bash\n# " + assert offset > len(head), "the old text is shorter than the new header" + return (head + "x" * (offset - len(head) - 1) + "\n" + + "printf 'new: read from the old offset\\n'; exit 7\n") + + +def source_tree(root: Path) -> Path: + """A copy of everything `--install` reads beside itself, for a test that + needs the NEW bytes to be its own. Run from a file, the command copies + what is beside it and fetches nothing, so the copy is complete or the run + would try the network, which this suite never reaches.""" + src = root / "src" + for rel in ("openRepoTools",) + FETCHED: + (src / rel).parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(REPO / rel, src / rel) + return src + + +def install_from(src: Path, home: Path) -> subprocess.CompletedProcess: + return subprocess.run(["bash", str(src / "openRepoTools"), "--install"], + capture_output=True, text=True, check=False, + input="", env=command_env(home)) + + +def placement_temporaries(home: Path) -> list[str]: + """Every `..openrepotools.*` left in a directory `--install` writes: + a temporary a placement made and never renamed or removed.""" + dirs = [home / ".local" / "bin", home / ".claude", + home / ".claude" / "commands", + home / ".claude-profiles" / "shared" / "commands"] + for name in SKILL_NAMES: + dirs += [home / ".claude" / "skills" / name, + home / ".claude-profiles" / "shared" / "skills" / name] + return sorted(str(p) for d in dirs if d.is_dir() for p in d.iterdir() + if ".openrepotools." in p.name) + + +def one_of_each_kind(home: Path) -> list[tuple[Path, Path, int]]: + """`(target, source, mode)` for a command, a skill and a command file, at + every destination each one has.""" + skill, command = SKILL_NAMES[0], COMMAND_NAMES[0] + return [ + (home / ".local" / "bin" / "lanes-edit.sh", REPO / "lanes-edit.sh", + 0o755), + (home / ".claude-profiles" / "shared" / "skills" / skill / "SKILL.md", + REPO / SKILL_PATHS[0], 0o644), + (home / ".claude" / "skills" / skill / "SKILL.md", + REPO / SKILL_PATHS[0], 0o644), + (home / ".claude-profiles" / "shared" / "commands" / f"{command}.md", + REPO / COMMAND_PATHS[0], 0o644), + (home / ".claude" / "commands" / f"{command}.md", + REPO / COMMAND_PATHS[0], 0o644), + ] + + +@NEEDS_JQ +def test_a_running_lane_tool_finishes_on_its_own_text_when_an_install_replaces_it( + tmp_path): + """THE DEFECT ITSELF, END TO END (#167). + + A lane tool is running from its installed path — `lanes-edit.sh` here, + the one whose register writes run longest — when `--install` replaces + it with newer bytes. The process must finish on the text it started with, + and the path must hold the new text afterwards. Written in place, the + process reads the new text from the old offset instead: it prints + `new: read from the old offset` and exits 7. + """ + src = source_tree(tmp_path) + home = tmp_path / "home" + first = install_from(src, home) + assert first.returncode == 0, first.stdout + first.stderr + name = "lanes-edit.sh" + target = home / ".local" / "bin" / name + target.write_text(OLD_RUNNING + OLD_TAIL, encoding="utf-8") + new = new_text_read_from(len(OLD_RUNNING)) + (src / name).write_text(new, encoding="utf-8") + + go = tmp_path / "go" + running = subprocess.Popen(["bash", str(target)], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, text=True, + env={**os.environ, "GO": str(go)}) + try: + assert running.stdout.readline() == "old: started\n" + second = install_from(src, home) + assert second.returncode == 0, second.stdout + second.stderr + assert f"{name}: updated at {target}" in second.stdout, second.stdout + go.touch() + out, err = running.communicate(timeout=120) + finally: + if running.poll() is None: + running.kill() + running.communicate() + assert (out, err, running.returncode) == ("old: finished\n", "", 0), ( + "the running copy did not finish on its own text — the install " + f"rewrote the file under it:\nstdout {out!r}\nstderr {err!r}\n" + f"exit {running.returncode}") + assert target.read_text(encoding="utf-8") == new + assert stat.S_IMODE(target.stat().st_mode) == 0o755 + assert placement_temporaries(home) == [] + + +@NEEDS_JQ +def test_every_placement_replaces_its_target_rather_than_rewriting_it(tmp_path): + """A NEW INODE AT EVERY PATH THAT CHANGED, AND THE OLD ONE LEFT WHOLE. + + One of each kind `--install` writes — a command, a skill and a command + file — at every destination each one has. Each is drifted, and held OPEN + the way a running process holds its script, before the install that + repairs it. Afterwards the path holds the right bytes at the right mode + under a different inode, and the handle still reads the drifted bytes + whole. In place, the inode is the same one and the handle reads the new + bytes. A `SKILL.md` executes nothing, but one way to place a file is one + rule to read, and a session that opens it mid-install sees the old text or + the new, never a truncated one. + """ + assert run_cmd("--install", home=tmp_path).returncode == 0 + held = [] + try: + for target, source, mode in one_of_each_kind(tmp_path): + drifted = target.read_bytes() + b"# drift\n" + target.write_bytes(drifted) + handle = target.open("rb") + held.append((target, source, mode, drifted, + os.fstat(handle.fileno()).st_ino, handle)) + result = run_cmd("--install", home=tmp_path) + assert result.returncode == 0, result.stdout + result.stderr + for target, source, mode, drifted, inode, handle in held: + assert f"updated at {target}" in result.stdout, target + assert target.stat().st_ino != inode, ( + f"{target} was rewritten in place: same inode {inode}") + assert handle.read() == drifted, ( + f"a reader holding the old {target} saw the new bytes") + assert target.read_bytes() == source.read_bytes(), target + assert stat.S_IMODE(target.stat().st_mode) == mode, target + finally: + for *_, handle in held: + handle.close() + assert placement_temporaries(tmp_path) == [] + + +@NEEDS_JQ +def test_an_install_that_changes_no_byte_renames_nothing(tmp_path): + """THE `cmp -s` SKIP STAYS (#167). A path whose bytes are already right + keeps its inode on a second run: renaming every file on every run would + turn an install that changed nothing into thirty-odd new inodes.""" + assert run_cmd("--install", home=tmp_path).returncode == 0 + targets = [tmp_path / ".local" / "bin" / n for n in INSTALLED] + targets += [t for t, _, _ in one_of_each_kind(tmp_path)] + before = {t: t.stat().st_ino for t in targets} + result = run_cmd("--install", home=tmp_path) + assert result.returncode == 0, result.stdout + result.stderr + assert {t: t.stat().st_ino for t in targets} == before + + +@pytest.mark.parametrize("tool, says", [ + ("cp", "could not write"), + ("chmod", "could not set mode 755 on"), + ("mv", "could not move the new"), +]) +@NEEDS_JQ +def test_a_placement_that_fails_leaves_the_old_copy_and_no_temporary( + tmp_path, tool, says): + """EACH STEP OF THE RENAME CAN FAIL, AND NONE OF THEM COSTS THE OLD COPY. + + A `cp` into the temporary, the mode stamp on it, or the `mv` over the + target is refused for ONE command by a fake first on `$PATH` that defers to + the real tool for everything else. The run refuses (2) naming the target, + the command already installed there keeps its bytes and its inode, no + temporary is left beside it, and the loop goes no further. + """ + real = shutil.which(tool) + if real is None: + pytest.skip(f"no real `{tool}` on PATH for the fake to defer to") + assert run_cmd("--install", home=tmp_path).returncode == 0 + bin_dir = tmp_path / ".local" / "bin" + name = INSTALLED[1] + target = bin_dir / name + old = target.read_bytes() + b"# an older copy\n" + target.write_bytes(old) + inode = target.stat().st_ino + shim = tmp_path / f"fake-{tool}" + shim.mkdir() + (shim / tool).write_text( + "#!/bin/sh\n" + 'for arg in "$@"; do\n' + '\tcase "$arg" in\n' + f'\t"{bin_dir}/.{name}.openrepotools."*)\n' + f"\t\tprintf 'fake {tool}: refused\\n' >&2\n" + "\t\texit 1 ;;\n" + "\tesac\n" + "done\n" + f'exec {real} "$@"\n', encoding="utf-8") + (shim / tool).chmod(0o755) + + result = run_cmd("--install", home=tmp_path, + env={"PATH": f"{shim}{os.pathsep}{os.environ['PATH']}"}) + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED:" in result.stderr, result.stderr + assert f"{says} {target}" in result.stderr, result.stderr + assert target.read_bytes() == old, "the old copy was not left as it was" + assert target.stat().st_ino == inode, "the old copy was replaced" + assert placement_temporaries(tmp_path) == [] + assert f"{INSTALLED[2]}: " not in result.stdout, ( + "the loop carried on past a placement it could not make") + + +def fake_owner_ls(root: Path, target: Path, uid: int, gid: int) -> Path: + """A `ls` first on `$PATH` that says `target` belongs to `uid:gid`, and is + the real one for every other path. OWNERSHIP IS WHAT THIS STANDS IN FOR, + as the fake `chmod` above stands in for it: no fixture here can make a file + another account owns, and `owner_of` asks `ls -lnd` and nothing else.""" + real = shutil.which("ls") + if real is None: + pytest.skip("no real `ls` on PATH for the fake to defer to") + shim = root / "fake-ls" + shim.mkdir() + (shim / "ls").write_text( + "#!/bin/sh\n" + 'last=""\n' + 'for arg in "$@"; do last="$arg"; done\n' + f'if [ "$last" = "{target}" ]; then\n' + f"\tprintf '%s\\n' '-rwxr-xr-x 1 {uid} {gid} 1 Jan 1 00:00 {target}'\n" + "\texit 0\n" + "fi\n" + f'exec {real} "$@"\n', encoding="utf-8") + (shim / "ls").chmod(0o755) + return shim + + +@NEEDS_JQ +def test_a_replaced_copy_is_handed_back_to_the_account_that_owned_it(tmp_path): + """THE OWNER STAYS WHO IT WAS (Copilot on #172). + + A rename puts the temporary's inode at the path, and the temporary belongs + to whoever ran the install — so a root `--install` into another user's + home would leave that user root-owned copies their own next `--install` + refuses as unwritable. `cp` kept the owner by keeping the inode. So where + the owner differs, the temporary is given the target's uid AND gid before + the rename: the `chown` is asked for exactly once, on the temporary, with + the old owner. A fake records it, because only root could make it. + """ + assert run_cmd("--install", home=tmp_path).returncode == 0 + bin_dir = tmp_path / ".local" / "bin" + name = INSTALLED[1] + target = bin_dir / name + target.write_bytes(target.read_bytes() + b"# drift\n") + uid, gid = os.getuid() + 4242, os.getgid() + 4243 + ls_shim = fake_owner_ls(tmp_path, target, uid, gid) + log = tmp_path / "chown.log" + chown_shim = tmp_path / "fake-chown" + chown_shim.mkdir() + (chown_shim / "chown").write_text( + "#!/bin/sh\n" + f"printf '%s\\n' \"$*\" >> '{log}'\n" + "exit 0\n", encoding="utf-8") + (chown_shim / "chown").chmod(0o755) + + result = run_cmd("--install", home=tmp_path, env={ + "PATH": os.pathsep.join([str(ls_shim), str(chown_shim), + os.environ["PATH"]])}) + assert result.returncode == 0, result.stdout + result.stderr + assert f"{name}: updated at {target}" in result.stdout, result.stdout + calls = log.read_text(encoding="utf-8").splitlines() + assert len(calls) == 1, calls + assert re.fullmatch( + rf"-- {uid}:{gid} {re.escape(str(bin_dir))}/\.{re.escape(name)}" + r"\.openrepotools\.\S+", calls[0]), calls + assert target.read_bytes() == (REPO / name).read_bytes() + assert placement_temporaries(tmp_path) == [] + + +@NOT_ROOT +@NEEDS_JQ +def test_a_copy_another_account_owns_is_refused_and_left_as_it_was(tmp_path): + """AND WHERE THE OWNER CANNOT BE KEPT, NOTHING IS TAKEN OVER (Copilot on + #172). An account that is not root may not give a file away, so the real + `chown` refuses, and a rename would leave the other account's file this + account's. The run refuses (2) naming the owner, the old copy keeps its + bytes and its inode, and no temporary is left beside it. + """ + assert run_cmd("--install", home=tmp_path).returncode == 0 + bin_dir = tmp_path / ".local" / "bin" + name = INSTALLED[1] + target = bin_dir / name + old = target.read_bytes() + b"# another account's copy\n" + target.write_bytes(old) + inode = target.stat().st_ino + uid = os.getuid() + 4242 + ls_shim = fake_owner_ls(tmp_path, target, uid, os.getgid() + 4243) + + result = run_cmd("--install", home=tmp_path, env={ + "PATH": f"{ls_shim}{os.pathsep}{os.environ['PATH']}"}) + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED:" in result.stderr, result.stderr + assert f"{target} belongs to uid {uid}" in result.stderr, result.stderr + assert target.read_bytes() == old + assert target.stat().st_ino == inode + assert placement_temporaries(tmp_path) == [] + assert f"{INSTALLED[2]}: " not in result.stdout, ( + "the loop carried on past a placement it refused") + + +@NEEDS_JQ +def test_a_copy_whose_owner_cannot_be_read_is_refused_and_left_as_it_was( + tmp_path): + """AN OWNER NOBODY COULD READ IS NOT "NOTHING TO KEEP" (Copilot on #172 at + bef4b85). `owner_of` answers nothing where `ls` cannot say, and an empty + answer is what an ABSENT target gives — so a target that EXISTS and whose + owner could not be read was renamed over with this run's own temporary, and + a root `--install` took the file over after all. A `ls` first on `$PATH` + that fails for that one path stands in for the unreadable owner: the run + refuses (2) naming the target, the old copy keeps its bytes and its inode, + and no temporary is left beside it. + """ + real = shutil.which("ls") + if real is None: + pytest.skip("no real `ls` on PATH for the fake to defer to") + assert run_cmd("--install", home=tmp_path).returncode == 0 + bin_dir = tmp_path / ".local" / "bin" + name = INSTALLED[1] + target = bin_dir / name + old = target.read_bytes() + b"# a copy whose owner cannot be read\n" + target.write_bytes(old) + inode = target.stat().st_ino + shim = tmp_path / "failing-ls" + shim.mkdir() + (shim / "ls").write_text( + "#!/bin/sh\n" + 'last=""\n' + 'for arg in "$@"; do last="$arg"; done\n' + f'if [ "$last" = "{target}" ]; then\n' + "\tprintf 'ls: cannot read %s\\n' \"$last\" >&2\n" + "\texit 2\n" + "fi\n" + f'exec {real} "$@"\n', encoding="utf-8") + (shim / "ls").chmod(0o755) + + result = run_cmd("--install", home=tmp_path, env={ + "PATH": f"{shim}{os.pathsep}{os.environ['PATH']}"}) + assert result.returncode == 2, result.stdout + result.stderr + assert "REFUSED:" in result.stderr, result.stderr + assert f"could not read who owns {target}" in result.stderr, result.stderr + assert target.read_bytes() == old + assert target.stat().st_ino == inode + assert placement_temporaries(tmp_path) == [] + assert f"{INSTALLED[2]}: " not in result.stdout, ( + "the loop carried on past a placement it refused") + + +@NEEDS_JQ +def test_a_group_that_cannot_be_kept_does_not_stop_the_placement(tmp_path): + """THE GROUP IS KEPT WHERE IT CAN BE, AND IS NO REASON TO REFUSE (Copilot + round 2 on #172). The owner is this account and the group is one it is not + in, so the real `chown` to that group is refused. At 755 and 644 the + group's bits are the other bits, so nobody gains or loses a permission by + it: the copy is placed, as `cp` placed it. + """ + assert run_cmd("--install", home=tmp_path).returncode == 0 + bin_dir = tmp_path / ".local" / "bin" + name = INSTALLED[1] + target = bin_dir / name + target.write_bytes(target.read_bytes() + b"# drift\n") + ls_shim = fake_owner_ls(tmp_path, target, os.getuid(), os.getgid() + 4243) + + result = run_cmd("--install", home=tmp_path, env={ + "PATH": f"{ls_shim}{os.pathsep}{os.environ['PATH']}"}) + assert result.returncode == 0, result.stdout + result.stderr + assert f"{name}: updated at {target}" in result.stdout, result.stdout + assert target.read_bytes() == (REPO / name).read_bytes() + assert placement_temporaries(tmp_path) == [] + + +#: THE TWO PLACES A SIGNAL CAN FIND A TEMPORARY. A slow `cp` holds the run +#: between `mktemp` and `mv`. A slow `mktemp` holds it AFTER the file is made +#: and BEFORE its name comes back — the window Copilot's round 2 on #172 named, +#: which a name learnt from `mktemp`'s answer could not cover. Each defers to +#: the real tool and is slow only for a temporary. +SLOW_TOOLS = { + "cp": ('for arg in "$@"; do\n' + '\tcase "$arg" in *.openrepotools.*) sleep 3 ;; esac\n' + 'done\n' + 'exec {real} "$@"\n'), + "mktemp": ('made=$({real} "$@") || exit $?\n' + 'case "$made" in *.openrepotools.*) sleep 3 ;; esac\n' + "printf '%s\\n' \"$made\"\n"), +} + + +@pytest.mark.parametrize("slow", sorted(SLOW_TOOLS)) +@pytest.mark.parametrize("signame, to_group", [ + ("SIGTERM", False), # a `kill` from another shell + ("SIGINT", True), # Ctrl-C: the whole foreground process group +]) +@NEEDS_JQ +def test_a_run_killed_mid_placement_leaves_no_temporary(tmp_path, signame, + to_group, slow): + """THE `EXIT` TRAP REMOVES WHAT A PLACEMENT HAS NOT YET RENAMED (#167). + + `place_by_rename` removes its temporary on every failure it sees; a signal + is one it cannot see, which is what `$PLACING` and the trap `workdir` + sets are for. A tool first on `$PATH` that is SLOW for a temporary holds + the run where one exists; the signal lands there. The run dies of it, and + nothing named `..openrepotools.*` is left beside a target. Without + the trap's clause every one of these leaves it behind, and with a trap that + knew only `mktemp`'s answer the Ctrl-C into a slow `mktemp` still did. + + THE CHILD STARTS WITH BOTH SIGNALS AT THEIR DEFAULT, as a foreground job at + a terminal has them. A runner started in the background by a + non-interactive shell (`nohup pytest … &`) has SIGINT IGNORED, an ignored + signal survives `exec`, and a shell may not un-ignore one it was started + with — so without this the case would measure the runner, not the trap. + """ + real = shutil.which(slow) + if real is None: + pytest.skip(f"no real `{slow}` on PATH for the slow one to defer to") + shim = tmp_path / f"slow-{slow}" + shim.mkdir() + (shim / slow).write_text( + "#!/bin/sh\n" + SLOW_TOOLS[slow].format(real=real), encoding="utf-8") + (shim / slow).chmod(0o755) + sig = getattr(signal, signame) + + def default_signals() -> None: + signal.signal(signal.SIGINT, signal.SIG_DFL) + signal.signal(signal.SIGTERM, signal.SIG_DFL) + + run = subprocess.Popen( + ["bash", str(COMMAND), "--install"], stdin=subprocess.DEVNULL, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, + env=command_env(tmp_path, { + "PATH": f"{shim}{os.pathsep}{os.environ['PATH']}"}), + start_new_session=True, preexec_fn=default_signals) + try: + deadline = time.monotonic() + 60 + while time.monotonic() < deadline and not placement_temporaries(tmp_path): + time.sleep(0.05) + assert placement_temporaries(tmp_path), ( + "no placement ever made a temporary for the signal to land beside") + if to_group: + os.killpg(run.pid, sig) + else: + run.send_signal(sig) + out, err = run.communicate(timeout=60) + finally: + if run.poll() is None: + os.killpg(run.pid, signal.SIGKILL) + run.communicate() + assert run.returncode in (-sig, 128 + sig), ( + f"the run did not die of {signame}: exit {run.returncode}\n{out}{err}") + assert placement_temporaries(tmp_path) == [] + + #: THE WORDS THIS INSTALLER USED TO PLACE AND DOES NOT ANY MORE. `restart` left #: under lane-collision-protocol Amendment 18 Addendum 2 (ratified #: 2026-09-14T16:50:32Z, verbatim "ratify"): `lane ` is that act, and the @@ -665,6 +1149,11 @@ def test_a_mode_stamp_the_bin_loop_cannot_make_reads_as_a_refusal(tmp_path): this suite answers a `gh` it cannot call and a `jq` it must do without. Ownership is what this stands in for; a fixture cannot make the ownership, and the stamp's own failure is the same failure either way. + + THE FAKE REFUSES THE ONE FILE BY EITHER NAME (#167): a copy that is being + WRITTEN is stamped on its temporary beside the target, before the rename, + so the target is never at the wrong mode — and a stamp that fails there + leaves no temporary behind. """ real = shutil.which("chmod") if real is None: @@ -676,10 +1165,11 @@ def test_a_mode_stamp_the_bin_loop_cannot_make_reads_as_a_refusal(tmp_path): (shim / "chmod").write_text( "#!/bin/sh\n" 'for arg in "$@"; do\n' - f'\tif [ "$arg" = "{refused}" ]; then\n' + '\tcase "$arg" in\n' + f'\t"{refused}"|"{bin_dir}/.{refused.name}.openrepotools."*)\n' "\t\tprintf 'fake chmod: this one is not ours to mode\\n' >&2\n" - "\t\texit 1\n" - "\tfi\n" + "\t\texit 1 ;;\n" + "\tesac\n" "done\n" f'exec {real} "$@"\n', encoding="utf-8") (shim / "chmod").chmod(0o755) @@ -693,6 +1183,8 @@ def test_a_mode_stamp_the_bin_loop_cannot_make_reads_as_a_refusal(tmp_path): "the run refused before it reached the file whose stamp fails") assert not (bin_dir / INSTALLED[2]).exists(), ( "the loop carried on past a mode stamp it could not make") + assert not refused.exists(), "a copy whose mode failed was placed anyway" + assert placement_temporaries(tmp_path) == [] assert not (tmp_path / ".claude").exists(), ( "the skills, the command files or the hook entries were placed by a " "run that refused")