From e05c2e90c0b6f6bf2b7ed241996eec59feb272d0 Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:58:02 +0000 Subject: [PATCH 1/4] --install writes every file beside its target and renames it over the target, so a lane tool running mid-install finishes on its own text (#167) `cp` onto an installed file kept its inode and rewrote its bytes, and bash reads a running script from its file at an offset, so a lane tool that was running when an install replaced it read the NEW text from the OLD offset (measured 2026-09-30: `A.sh: line 4: ript,: command not found`). `place_by_rename` writes each placement to a `mktemp` temporary in the target's own directory, stamps its mode there, refuses a directory at the target, then `mv -f`s it over the target. Every failure removes the temporary and refuses naming the target, leaving the old copy untouched; `$PLACING` and the EXIT trap remove it on a signal. The `cmp -s` skip stays and stamps the mode in place. Used for the sixteen bin files, the skills and the command files. The planners' refusals keep refusing a symlink, a directory and a read-only file, with their reasons restated for a rename. README.md is left alone here: its install paragraph is being rewritten by the sibling PRs #168 and #169, so its one sentence about `cp` following a symlink is corrected after they land rather than in a conflicting hunk. Tests: a running lane tool finishes on its own text across an install; a new inode at every changed path while an open handle keeps the old bytes; an unchanged install renames nothing; a failing cp, chmod or mv leaves the old copy and no temporary; a run killed by SIGTERM or SIGINT mid-placement leaves no temporary; and the function itself on each platform's bash, including Git Bash on Windows. Lane: openxfactory-5 (openXfactory-5) Co-Authored-By: Claude Opus 5.5 --- openRepoTools | 197 ++++++++++++----- tests/test_install_by_rename.py | 177 ++++++++++++++++ tests/test_openrepotools_command.py | 318 +++++++++++++++++++++++++++- 3 files changed, 634 insertions(+), 58 deletions(-) create mode 100644 tests/test_install_by_rename.py diff --git a/openRepoTools b/openRepoTools index 3cb5293..365c958 100755 --- a/openRepoTools +++ b/openRepoTools @@ -71,6 +71,11 @@ SELF="${BASH_SOURCE[0]:-$0}" MODE="" WORKDIR="" STAGED="" +# THE TEMPORARY A PLACEMENT HAS MADE AND NOT YET RENAMED (#167), named here for +# the `EXIT` trap `workdir` sets: `place_by_rename` removes it on every failure +# it sees, and this is what removes it on the ones it cannot see — a signal, or +# an `errexit` between its `mktemp` and its `mv`. +PLACING="" say() { printf '%s\n' "$*"; } die() { printf '\nREFUSED: %s\n' "$1" >&2; exit "${2:-2}"; } @@ -217,7 +222,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 +984,59 @@ mode_is() { # [ -n "$(find "$2" -maxdepth 0 -perm "$1" -print 2>/dev/null)" ] } +# 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 +# `$PLACING` names it in between for the `EXIT` trap, so a run killed between the +# `mktemp` and the `mv` leaves nothing behind either. 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/`). +# +# 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="" + tmp="$(mktemp -- "$(dirname -- "$target")/.${target##*/}.openrepotools.XXXXXX")" || + die "could not create a temporary file beside $target, so it was not placed. + $rest" + PLACING="$tmp" + 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"; } + 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 +1080,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 +1395,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 +1422,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 +1435,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 +1450,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 +1469,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 +1497,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 +1508,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 +1630,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 +1912,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 +1930,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 +1961,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..e6df505 --- /dev/null +++ b/tests/test_install_by_rename.py @@ -0,0 +1,177 @@ +# 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` and coreutils, which is why it can be run on its 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 placement_function() -> str: + """`place_by_rename` as the installer carries it, cut out of the script.""" + text = COMMAND.read_text(encoding="utf-8") + match = re.search(r"^place_by_rename\(\) \{[^\n]*\n.*?^\}\n", text, + re.MULTILINE | re.DOTALL) + assert match, "`place_by_rename() {` … `}` 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" + + placement_function() + + '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..9ba2e5a 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,308 @@ 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") + + +@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): + """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 `cp` first on `$PATH` that is SLOW into a temporary holds + the run between its `mktemp` and its `mv`; 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. + + 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("cp") + if real is None: + pytest.skip("no real `cp` on PATH for the slow one to defer to") + shim = tmp_path / "slow-cp" + shim.mkdir() + (shim / "cp").write_text( + "#!/bin/sh\n" + 'for arg in "$@"; do\n' + '\tcase "$arg" in *.openrepotools.*) sleep 3 ;; esac\n' + "done\n" + f'exec {real} "$@"\n', encoding="utf-8") + (shim / "cp").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 +969,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 +985,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 +1003,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") From 0043d37838b5716cd3316f26a969789eb91ea0ba Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 5 Oct 2026 22:15:04 +0000 Subject: [PATCH 2/4] A replaced copy keeps its owner: the temporary takes the target's uid and gid before the rename, and a copy another account owns is refused rather than taken over (Copilot on #172) A rename puts the temporary's inode at the path, and the temporary belongs to whoever runs the install, so a root --install into another user's home would have left that user root-owned copies their own next --install refuses as unwritable. `cp` kept the owner by keeping the inode. `place_by_rename` now asks `owner_of` (`ls -lnd`, no `stat`) for the target's uid:gid and, where the owner differs from the temporary's, gives the temporary that owner before the rename. Where that `chown` is refused, the placement removes the temporary and refuses, leaving the old copy as it was. The cross-platform harness carries `owner_of` beside the function. Tests: a fake `ls` stands in for another account's ownership. A recording fake `chown` shows the temporary handed back to uid:gid exactly once, and the real `chown` (not root) shows the refusal with the old copy, its inode and no temporary left. Lane: openxfactory-5 (openXfactory-5) Co-Authored-By: Claude Opus 5.5 --- openRepoTools | 29 ++++++++- tests/test_install_by_rename.py | 14 +++-- tests/test_openrepotools_command.py | 96 +++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 7 deletions(-) diff --git a/openRepoTools b/openRepoTools index 365c958..bd6d32d 100755 --- a/openRepoTools +++ b/openRepoTools @@ -984,6 +984,15 @@ 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 @@ -1009,12 +1018,23 @@ mode_is() { # # 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. Asked only where the OWNER differs: +# a group that differs costs the owner nothing. Where that `chown` is refused — +# an account placing over a file another account owns, which only root may +# give away — the placement refuses rather than take the file over. +# # 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="" + local source="$1" target="$2" mode="$3" rest="$4" tmp="" was="" now="" tmp="$(mktemp -- "$(dirname -- "$target")/.${target##*/}.openrepotools.XXXXXX")" || die "could not create a temporary file beside $target, so it was not placed. $rest" @@ -1025,6 +1045,13 @@ place_by_rename() { # 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 [ -n "$was" ] && [ "${was%%:*}" != "${now%%:*}" ]; then + chown -- "$was" "$tmp" 2>/dev/null || + { 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="" diff --git a/tests/test_install_by_rename.py b/tests/test_install_by_rename.py index e6df505..af2a484 100644 --- a/tests/test_install_by_rename.py +++ b/tests/test_install_by_rename.py @@ -15,7 +15,8 @@ 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` and coreutils, which is why it can be run on its own. +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 @@ -81,12 +82,12 @@ def posix(path: Path) -> str: return path.as_posix() -def placement_function() -> str: - """`place_by_rename` as the installer carries it, cut out of the script.""" +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(r"^place_by_rename\(\) \{[^\n]*\n.*?^\}\n", text, + match = re.search(rf"^{re.escape(name)}\(\) \{{[^\n]*\n.*?^\}}\n", text, re.MULTILINE | re.DOTALL) - assert match, "`place_by_rename() {` … `}` is no longer in openRepoTools" + assert match, f"`{name}() {{` … `}}` is no longer in openRepoTools" return match.group(0) @@ -98,7 +99,8 @@ def place(tmp_path: Path, source: Path, target: Path, "set -euo pipefail\n" 'PLACING=""\n' "die() { printf 'REFUSED: %s\\n' \"$1\" >&2; exit 2; }\n" - + placement_function() + + 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), diff --git a/tests/test_openrepotools_command.py b/tests/test_openrepotools_command.py index 9ba2e5a..b081003 100644 --- a/tests/test_openrepotools_command.py +++ b/tests/test_openrepotools_command.py @@ -681,6 +681,102 @@ def test_a_placement_that_fails_leaves_the_old_copy_and_no_temporary( "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") + + @pytest.mark.parametrize("signame, to_group", [ ("SIGTERM", False), # a `kill` from another shell ("SIGINT", True), # Ctrl-C: the whole foreground process group From f85e6181dfc71665547d47c2e8eaeb6796bb15ee Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Mon, 5 Oct 2026 22:26:17 +0000 Subject: [PATCH 3/4] The trap knows a temporary's name before mktemp makes it, and a group that cannot be kept does not stop a placement (Copilot round 2 on #172) A signal that arrived after `mktemp` had made the temporary and before its name was assigned to `$PLACING` left the file behind, because the trap knew only `mktemp`'s answer. `$PLACING` is now the temporary's PREFIX, `..openrepotools..`, set before `mktemp` runs, and the EXIT trap removes `"$PLACING"*`. Reproduced with a slow `mktemp` that makes the file and stalls before printing its name: SIGTERM to the run and SIGINT to its group both left the temporary at the previous head and leave nothing now. The owner rule now tries to keep the group as well. Where only the group differs and cannot be kept, the copy is placed anyway: at 755 and 644 the group bits equal the other bits, so no permission moves. A differing owner that cannot be kept is still a refusal. Lane: openxfactory-5 (openXfactory-5) Co-Authored-By: Claude Opus 5.5 --- openRepoTools | 57 ++++++++++++++---------- tests/test_openrepotools_command.py | 67 ++++++++++++++++++++++------- 2 files changed, 87 insertions(+), 37 deletions(-) diff --git a/openRepoTools b/openRepoTools index bd6d32d..4a7fce1 100755 --- a/openRepoTools +++ b/openRepoTools @@ -71,10 +71,12 @@ SELF="${BASH_SOURCE[0]:-$0}" MODE="" WORKDIR="" STAGED="" -# THE TEMPORARY A PLACEMENT HAS MADE AND NOT YET RENAMED (#167), named here for -# the `EXIT` trap `workdir` sets: `place_by_rename` removes it on every failure -# it sees, and this is what removes it on the ones it cannot see — a signal, or -# an `errexit` between its `mktemp` and its `mv`. +# 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' "$*"; } @@ -222,7 +224,7 @@ USAGE workdir() { if [ -z "$WORKDIR" ]; then WORKDIR="$(mktemp -d)" - trap 'rm -rf -- "$WORKDIR"; [ -z "$PLACING" ] || rm -f -- "$PLACING"' EXIT + trap 'rm -rf -- "$WORKDIR"; [ -z "$PLACING" ] || rm -f -- "$PLACING"*' EXIT fi } @@ -1012,11 +1014,16 @@ owner_of() { # # 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 -# `$PLACING` names it in between for the `EXIT` trap, so a run killed between the -# `mktemp` and the `mv` leaves nothing behind either. 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/`). +# 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 @@ -1024,10 +1031,13 @@ owner_of() { # # 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. Asked only where the OWNER differs: -# a group that differs costs the owner nothing. Where that `chown` is refused — -# an account placing over a file another account owns, which only root may -# give away — the placement refuses rather than take the file over. +# 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). # # 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, @@ -1035,10 +1045,10 @@ owner_of() { # # 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="" - tmp="$(mktemp -- "$(dirname -- "$target")/.${target##*/}.openrepotools.XXXXXX")" || - die "could not create a temporary file beside $target, so it was not placed. - $rest" - PLACING="$tmp" + 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"; } @@ -1047,10 +1057,13 @@ place_by_rename() { # $rest"; } was="$(owner_of "$target")" now="$(owner_of "$tmp")" - if [ -n "$was" ] && [ "${was%%:*}" != "${now%%:*}" ]; then - chown -- "$was" "$tmp" 2>/dev/null || - { 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"; } + 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" diff --git a/tests/test_openrepotools_command.py b/tests/test_openrepotools_command.py index b081003..827bb55 100644 --- a/tests/test_openrepotools_command.py +++ b/tests/test_openrepotools_command.py @@ -777,21 +777,62 @@ def test_a_copy_another_account_owns_is_refused_and_left_as_it_was(tmp_path): "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): + 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 `cp` first on `$PATH` that is SLOW into a temporary holds - the run between its `mktemp` and its `mv`; 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. + 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 @@ -799,18 +840,14 @@ def test_a_run_killed_mid_placement_leaves_no_temporary(tmp_path, signame, 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("cp") + real = shutil.which(slow) if real is None: - pytest.skip("no real `cp` on PATH for the slow one to defer to") - shim = tmp_path / "slow-cp" + pytest.skip(f"no real `{slow}` on PATH for the slow one to defer to") + shim = tmp_path / f"slow-{slow}" shim.mkdir() - (shim / "cp").write_text( - "#!/bin/sh\n" - 'for arg in "$@"; do\n' - '\tcase "$arg" in *.openrepotools.*) sleep 3 ;; esac\n' - "done\n" - f'exec {real} "$@"\n', encoding="utf-8") - (shim / "cp").chmod(0o755) + (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: From 1905b7984c1f9877569c20971ffcf9ebd08b3e1c Mon Sep 17 00:00:00 2001 From: Brett Heap <1513478+brettheap@users.noreply.github.com> Date: Tue, 6 Oct 2026 14:10:52 +0000 Subject: [PATCH 4/4] An existing target whose owner cannot be read is refused rather than renamed over (Copilot on #172 at bef4b85) `owner_of` answers nothing where `ls` cannot say, and an empty answer is also what an absent target gives, so `place_by_rename` read a target that EXISTS with an unreadable owner as one with nothing to keep, and a root --install could rename its own temporary over it after all. Such a target is now refused before the rename: the temporary is removed, the old copy keeps its bytes and its inode, and the refusal names the target. Test first: a `ls` that fails for the one target exited 0 and replaced it at bef4b85, and is refused (2) now. Lane: openxfactory-5 (openXfactory-5) Co-Authored-By: Claude Opus 5.5 --- openRepoTools | 12 +++++++- tests/test_openrepotools_command.py | 47 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/openRepoTools b/openRepoTools index 4a7fce1..69f9f32 100755 --- a/openRepoTools +++ b/openRepoTools @@ -1037,7 +1037,11 @@ owner_of() { # # 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). +# 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, @@ -1057,6 +1061,12 @@ place_by_rename() { # $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 diff --git a/tests/test_openrepotools_command.py b/tests/test_openrepotools_command.py index 827bb55..8feb7cf 100644 --- a/tests/test_openrepotools_command.py +++ b/tests/test_openrepotools_command.py @@ -777,6 +777,53 @@ def test_a_copy_another_account_owns_is_refused_and_left_as_it_was(tmp_path): "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