diff --git a/install.sh b/install.sh index ee559ea89..6954ce6bc 100755 --- a/install.sh +++ b/install.sh @@ -221,6 +221,11 @@ echo "" # --- Update mode --- if [ "$UPDATE_ONLY" = true ]; then + # Captured before CMD_NAME gets defaulted/resolved below, so it still means + # "the caller typed --cmd" specifically (#553's shim-force decision needs + # exactly that, not "we ended up with some skill name one way or another"). + CMD_WAS_EXPLICIT=false + [ -n "$CMD_NAME" ] && CMD_WAS_EXPLICIT=true # Find existing install. If --cmd was passed, update exactly that skill; # otherwise preserve the historical "first installed agmsg skill" behavior. if [ -n "$CMD_NAME" ]; then @@ -320,10 +325,42 @@ if [ "$UPDATE_ONLY" = true ]; then # types/ -> scripts/drivers/types/ move. Re-running install regenerates it with # the new path; install is idempotent and overwrites only an agmsg shim (a # user's own codex binary fails is_agmsg_shim and is left untouched). + # + # Forced ONLY when the caller typed --cmd (CMD_WAS_EXPLICIT, captured above + # before CMD_NAME could be defaulted/resolved to anything else): that is + # the documented recovery path for #553 (a different install's --cmd having + # clobbered the shim), and naming the target explicitly is what makes + # reclaiming it safe. Bare `--update` (no --cmd) resolves SKILL_DIR by + # scanning for an existing install WITHOUT failing closed on more than one + # candidate on this base (#599; the fail-closed fix is PR #659, not yet + # merged here) -- so on a multi-install machine, bare `--update` today can + # land on an install the caller never named at all. Forcing unconditionally + # would let THAT arbitrarily-selected install steal the shim from another + # one, compounding #599 with a #553-shaped consequence (review finding). + # Not forcing means bare `--update` still refreshes a shim this SAME + # install already owns (the common single-install case, unaffected either + # way) but no longer silently reaches past a shim someone else owns. + # + # Capture status into a variable rather than piping it straight into + # `grep -q` (measured, not theoretical): status now prints a second "owner:" + # line (#553), and `grep -q` exits the instant it matches the first line, + # closing its end of the pipe. status's own `echo` of the second line then + # hits a reader that is already gone -- SIGPIPE, a nonzero exit for that + # stage -- and under this script's `pipefail`, that alone flips the whole + # `if` to false even though grep DID match. A one-line status (as this had + # before #553) never triggers it: there is no second write for the closed + # pipe to reject. Capturing first reads status to completion regardless of + # how many lines it prints, so growing its output again later can't reopen + # this. CODEX_SHIM="$SKILL_DIR/scripts/drivers/types/codex/codex-shim-install.sh" - if [ -x "$CODEX_SHIM" ] && AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" status 2>/dev/null | grep -q '^installed:'; then - AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" install >/dev/null 2>&1 \ - && echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" + CODEX_SHIM_STATUS="" + [ -x "$CODEX_SHIM" ] && CODEX_SHIM_STATUS="$(AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" status 2>/dev/null || true)" + if printf '%s' "$CODEX_SHIM_STATUS" | grep -q '^installed:'; then + CODEX_SHIM_FORCE="" + [ "$CMD_WAS_EXPLICIT" = true ] && CODEX_SHIM_FORCE=1 + if AGMSG_CODEX_SHIM_INSTALL_QUIET=1 AGMSG_CODEX_SHIM_FORCE="$CODEX_SHIM_FORCE" "$CODEX_SHIM" install >/dev/null; then + echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" + fi fi install_windows_helpers INSTALLED_VERSION="$(agmsg_source_version)" @@ -390,11 +427,24 @@ cp "$SCRIPT_DIR/openai.yaml" "$SKILL_DIR/agents/openai.yaml" 2>/dev/null || true chmod +x "$SKILL_DIR/scripts/"*.sh chmod +x "$SKILL_DIR/scripts/drivers/types/codex/"*.sh 2>/dev/null || true # Re-point an existing Codex monitor shim at the new path on a reinstall over an -# older layout (no-op when no agmsg shim is present). See the --update block above. +# older layout (no-op when no agmsg shim is present). See the --update block +# above. NOT forced (#553): unlike --update, a fresh install here gives no +# signal that the caller means to take over an EXISTING install's shim, so a +# --cmd for a second/different name must not silently repoint it away from +# whichever install already owns it. codex-shim-install.sh itself refuses that +# and says whose it is; surface that here instead of swallowing it. CODEX_SHIM="$SKILL_DIR/scripts/drivers/types/codex/codex-shim-install.sh" -if [ -x "$CODEX_SHIM" ] && AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" status 2>/dev/null | grep -q '^installed:'; then - AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" install >/dev/null 2>&1 \ - && echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" +CODEX_SHIM_STATUS="" +[ -x "$CODEX_SHIM" ] && CODEX_SHIM_STATUS="$(AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" status 2>/dev/null || true)" +if printf '%s' "$CODEX_SHIM_STATUS" | grep -q '^installed:'; then + # Stdout suppressed (mirrors the --update block's success case above); + # stderr is NOT, since codex-shim-install.sh's own refusal already names the + # current owner and the exact consequence of forcing -- repeating a + # shorter, separate version of that here would risk saying something + # different from what actually happens. + if AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" install >/dev/null; then + echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" + fi fi install_windows_helpers diff --git a/scripts/drivers/types/codex/codex-shim-install.sh b/scripts/drivers/types/codex/codex-shim-install.sh index a8a67b982..30d1ab1af 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -28,6 +28,114 @@ shell_quote() { printf '%q' "$1" } +# Lists this machine's agmsg install candidates, one per line: every +# directory under ~/.agents/skills carrying a `.agmsg` marker file. Derives +# the set directly from this machine's actual state -- not from whether +# #599's fail-closed multi-install handling (PR #659) happens to be merged +# on whatever branch calls this, which it may not be (review finding: +# measure the base, don't assume another PR landed). +# +# Kept separate from agmsg_only_one_install below so #659's own bare- +# `--update` candidate enumeration -- not written yet, #659 is still open +# against `main` and unmerged -- has something to consume instead of +# re-scanning ~/.agents/skills a second time, once the two share a base. +# Until #659 lands, this duplicates (rather than shares) that logic; this +# is the known, accepted overlap flagged in review. +agmsg_install_candidates() { + local skills_dir d + skills_dir="$(dirname "$AGENTS_BIN")/skills" + [ -d "$skills_dir" ] || return 0 + for d in "$skills_dir"/*/; do + [ -f "${d}.agmsg" ] || continue + printf '%s\n' "${d%/}" + done +} + +# True iff no agmsg install OTHER THAN this one (SCRIPT_DIR) is among +# agmsg_install_candidates. +# +# What this buys: a legacy shim (agmsg's, but predating ownership tracking, +# #553) has no recorded owner to compare against -- but if this is +# PROVABLY the only agmsg install on the machine, nothing else could have +# written it, so claiming it needs no --cmd/--force to be safe. With two or +# more installs present, this returns false and the caller falls back to +# fail-closed, same as an unrecorded owner always has since #553. +# +# Counts "other than me" rather than a plain candidate-count check for +# exactly one, and treats myself as a candidate whether or not my own +# .agmsg marker is on disk yet (review finding): install.sh's fresh --cmd +# path checks/refreshes the shim BEFORE it touches this install's own +# marker, so during a fresh install this install's marker genuinely does +# not exist yet. A plain candidate count would then see only the FIRST +# install's marker, conclude "only one install exists", and let a second, +# distinctly different install silently claim a legacy shim without --force +# -- the very bug this file exists to prevent. +# +# agmsg_install_candidates lists skill-root directories (one level under +# ~/.agents/skills), but SCRIPT_DIR is the nested .../scripts/drivers/types/ +# codex directory beneath one -- comparing SCRIPT_DIR itself against those +# entries would never match, so strip the fixed suffix install.sh always +# lays this script out under to recover my own skill root first. +# +# The implicit "I count as a candidate" exception is granted ONLY when that +# recovered self_root actually sits directly under ~/.agents/skills -- i.e. +# SCRIPT_DIR really has the shape install.sh lays this script out under. +# Without that check, running this script directly against some unrelated +# location (as several of this file's own tests do, standing in for "an +# install" without a real ~/.agents/skills tree at all) would trivially +# satisfy "zero others" the moment ~/.agents/skills is empty or absent -- +# self can't be trusted as a real, soon-to-register install just because it +# also doesn't show up as an "other". In that fallback case this reverts to +# the plain, pre-self-aware question: is there exactly one REAL marked +# candidate on disk, full stop. +# +# awk (not `grep -v | wc -l`) because it always exits 0, so a count of zero +# others never trips this script's `set -o pipefail` the way a no-match +# grep would. +agmsg_only_one_install() { + local skills_dir self_root + skills_dir="$(dirname "$AGENTS_BIN")/skills" + self_root="${SCRIPT_DIR%/scripts/drivers/types/codex}" + if [ "$SCRIPT_DIR" != "$self_root" ] && [ "$(dirname "$self_root")" = "$skills_dir" ]; then + local other_count + other_count="$(agmsg_install_candidates | awk -v self="$self_root" '$0 != self {c++} END {print c + 0}')" + [ "$other_count" -eq 0 ] + else + local count + count="$(agmsg_install_candidates | wc -l | tr -d ' ')" + [ "$count" -eq 1 ] + fi +} + +# Prints the shell-QUOTED (%q) skill script dir baked into the currently- +# installed shim -- empty if there is none, the shim is not an agmsg shim, or +# it predates ownership tracking (#553; see below). This is the install that +# "owns" the shim: the one whose codex-shim.sh it execs into and whose +# storage/drivers every Codex launch through it will resolve. +# +# Reads a DEDICATED comment line (`# agmsg-shim-owner: <%q-quoted dir>`, +# written by `install` below) via plain string extraction -- never eval, and +# never the executable `export AGMSG_CODEX_SHIM_SCRIPT_DIR=...` line the shim +# itself needs at runtime. is_agmsg_shim matching only proves the marker +# STRING is present; it says nothing about the rest of the file's contents, +# which on a local, single-user path like this one could have been hand- +# edited after the fact. Evaling anything sourced from it -- as an earlier +# version of this function did -- turns a read-only `status` call into an +# arbitrary-code-execution path for whoever can write $TARGET (security +# review finding). Plain text extraction has no such path regardless of what +# the line contains. +# +# Quoted, not raw, on this side too (every caller compares/prints this value +# against another %q-quoted value, never an unquoted path) -- a path +# containing a literal newline would otherwise let its OWN content forge a +# second, fake comment line (security review, non-blocking but cheap to +# close). Neither side ever needs the literal path back, only equality and +# display, both of which a consistently-quoted value still gives correctly. +shim_owner_script_dir() { + is_agmsg_shim || return 0 + sed -n 's/^# agmsg-shim-owner: //p' "$TARGET" 2>/dev/null | head -1 +} + cmd="${1:-function}" case "$cmd" in -h|--help) @@ -48,12 +156,58 @@ EOF echo "codex-shim-install: move it aside or remove it first" >&2 exit 1 fi + # The shim path is one file shared by every install on the machine (#553): + # whichever install's `install` ran last wins, and every Codex launch + # through the shim then dispatches into ITS drivers/storage — silently, + # since nothing here previously recorded whose the existing one was. + # + # A shim that IS ours (is_agmsg_shim, checked above) but carries no + # `# agmsg-shim-owner:` line at all is not evidence of "unowned" — it is + # every shim this tool ever wrote before this check existed. Treating + # "no owner recorded" as "safe to take" would silently repeat #553's own + # bug for exactly the migration moment it matters most: the first time a + # second-named install runs an installer that HAS this fix, against a + # production shim that does not (review finding). Both a foreign-owned + # and an owner-unknown shim fail closed here; only a shim already + # recording THIS install's own SCRIPT_DIR skips the guard. + self_owner="$(shell_quote "$SCRIPT_DIR")" + owner="$(shim_owner_script_dir)" + # An owner-unknown (legacy) shim is claimable WITHOUT --force when this is + # provably the only agmsg install on the machine: nothing else could have + # written it, so there is no one to take it from. This is what keeps the + # ordinary, single-install upgrade path working -- most real machines, + # migrating from a shim written before ownership tracking existed -- + # without reopening the multi-install theft this whole guard exists to + # close (review finding: the two fixes above, each correct alone, combined + # to block the routine case they were never meant to touch). + legacy_but_sole_install=false + [ -z "$owner" ] && agmsg_only_one_install && legacy_but_sole_install=true + if [ -e "$TARGET" ] && [ "$owner" != "$self_owner" ] && [ "${AGMSG_CODEX_SHIM_FORCE:-}" != "1" ] \ + && [ "$legacy_but_sole_install" != true ]; then + if [ -n "$owner" ]; then + echo "codex-shim-install: $TARGET is owned by a different install:" >&2 + echo " $owner" >&2 + else + echo "codex-shim-install: $TARGET is an agmsg shim from before ownership tracking (#553)," >&2 + echo "codex-shim-install: so which install actually owns it cannot be named." >&2 + fi + echo "codex-shim-install: refusing to repoint it at $SCRIPT_DIR" >&2 + echo "codex-shim-install: to claim it for THIS install instead, re-run with AGMSG_CODEX_SHIM_FORCE=1 --" >&2 + echo "codex-shim-install: every Codex launch that goes through the shim will then dispatch into $SCRIPT_DIR" >&2 + exit 1 + fi { echo "#!/usr/bin/env bash" echo "set -euo pipefail" echo "" echo "# Optional Codex entrypoint shim for agmsg monitor mode." echo "# Generated by agmsg. Dispatches to the installed skill script." + # Comment, never executed: the ownership marker shim_owner_script_dir + # reads with plain text extraction, not eval. %q-quoted (not the raw + # path) so a path containing a literal newline can't forge a second, + # fake comment line of its own -- every reader compares/prints this + # quoted, never unquotes it back to a real path. + echo "# agmsg-shim-owner: $self_owner" echo "" echo "export AGMSG_CODEX_SHIM_WRAPPER=1" echo "export AGMSG_CODEX_SHIM_SCRIPT_DIR=$(shell_quote "$SCRIPT_DIR")" @@ -85,6 +239,19 @@ EOF status) if is_agmsg_shim; then echo "installed: $TARGET" + # A second, separate line, not appended to the first. The "installed:" + # prefix a caller matches on is unchanged either way, but a caller + # piping this straight into `grep -q` (rather than capturing it first) + # can still break under `pipefail` once there is a second line at all — + # see install.sh's own callers for why, and capture before grep there. + owner="$(shim_owner_script_dir)" + if [ "$owner" = "$(shell_quote "$SCRIPT_DIR")" ]; then + echo "owner: this install ($SCRIPT_DIR)" + elif [ -n "$owner" ]; then + echo "owner: a different install ($owner)" + else + echo "owner: unknown (predates ownership tracking, #553)" + fi else echo "not installed: $TARGET" fi diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 09ebbce02..e3daad35c 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -248,3 +248,243 @@ teardown() { [[ "$output" =~ "cannot find delivery.sh" ]] grep -q "real-codex" "$CALL_LOG" } + +# --- shim ownership (#553): install.sh's own driver is tested separately in +# test_install.bats; these exercise codex-shim-install.sh's own refuse/force +# logic directly, against two independent copies of the codex driver dir +# standing in for two different installs. + +_second_codex_dir() { + local dir="$TEST_PROJECT/second-install/codex" + mkdir -p "$dir" + cp -R "$TYPES/codex/." "$dir/" + printf '%s' "$dir" +} + +@test "codex shim install: refuses to repoint a shim owned by a different install" { + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME" + bash "$TYPES/codex/codex-shim-install.sh" install >/dev/null + local shim="$HOME/.agents/bin/codex" + local before; before="$(cat "$shim")" + + local second_dir; second_dir="$(_second_codex_dir)" + run bash "$second_dir/codex-shim-install.sh" install + [ "$status" -ne 0 ] + printf '%s' "$output" | grep -qF "owned by a different install" + printf '%s' "$output" | grep -qF "$TYPES/codex" # names the actual owner, not just "someone else" + printf '%s' "$output" | grep -qF "AGMSG_CODEX_SHIM_FORCE=1" + + [ "$(cat "$shim")" = "$before" ] # byte-for-byte unchanged +} + +@test "codex shim install: AGMSG_CODEX_SHIM_FORCE=1 reclaims a shim owned by a different install" { + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME" + bash "$TYPES/codex/codex-shim-install.sh" install >/dev/null + + local second_dir; second_dir="$(_second_codex_dir)" + run bash -c "AGMSG_CODEX_SHIM_FORCE=1 bash '$second_dir/codex-shim-install.sh' install" + [ "$status" -eq 0 ] + grep -q "AGMSG_CODEX_SHIM_SCRIPT_DIR=$second_dir" "$HOME/.agents/bin/codex" +} + +@test "codex shim status: names which install currently owns the shim" { + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME" + bash "$TYPES/codex/codex-shim-install.sh" install >/dev/null + + run bash "$TYPES/codex/codex-shim-install.sh" status + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "owner: this install ($TYPES/codex)" + + local second_dir; second_dir="$(_second_codex_dir)" + run bash "$second_dir/codex-shim-install.sh" status + [ "$status" -eq 0 ] + [[ "$output" == *"owner: a different install ($TYPES/codex)"* ]] +} + +@test "codex shim install: a plain, non-agmsg codex binary is still refused regardless of ownership wording (#553 regression guard)" { + # The pre-existing is_agmsg_shim guard, unrelated to ownership -- a real + # user codex binary at the target path must never be touched or described + # as "owned by a different install" (that phrasing is reserved for a shim + # this tool itself generated). + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME/.agents/bin" + printf '#!/usr/bin/env bash\necho real\n' > "$HOME/.agents/bin/codex" + chmod +x "$HOME/.agents/bin/codex" + local before; before="$(cat "$HOME/.agents/bin/codex")" + + run bash "$TYPES/codex/codex-shim-install.sh" install + [ "$status" -ne 0 ] + printf '%s' "$output" | grep -qF "refusing to overwrite existing" + refute grep -qF "owned by a different install" <(printf '%s' "$output") + [ "$(cat "$HOME/.agents/bin/codex")" = "$before" ] +} + +@test "codex shim status: a multi-line status output does not spuriously fail a piped grep -q check (#553 regression guard)" { + # Measured, not theoretical: adding the second "owner:" line to status's + # output broke install.sh's own \`status ... | grep -q '^installed:'\` check + # under this script's \`pipefail\` -- grep exits the instant it matches the + # first line, and status's still-pending write of the second line then hits + # SIGPIPE, which pipefail reports as the pipeline failing even though grep + # DID match. Pins the exact shape install.sh now avoids by capturing status + # into a variable first; this test guards the underlying hazard directly so + # a future caller that pipes status straight into grep -q reintroduces it. + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME" + bash "$TYPES/codex/codex-shim-install.sh" install >/dev/null + + run bash -c "set -o pipefail; bash '$TYPES/codex/codex-shim-install.sh' status | grep -q '^installed:'" + [ "$status" -ne 0 ] # documents the hazard: this form is expected to fail today + + run bash -c "set -o pipefail; out=\"\$(bash '$TYPES/codex/codex-shim-install.sh' status)\"; printf '%s' \"\$out\" | grep -q '^installed:'" + [ "$status" -eq 0 ] # the capture-first form install.sh actually uses does not +} + +@test "codex shim status/install: a tampered shim cannot execute code via ownership parsing (#553 security regression guard)" { + # is_agmsg_shim's authenticity check is a grep for one marker string -- it + # says nothing about the rest of a LOCAL, single-user, world-writable-by- + # that-user path having been hand-edited afterward. An earlier version of + # shim_owner_script_dir read the ownership line via `eval`, which turned + # that gap into arbitrary code execution reachable from a read-only `status` + # call. Plants a shim carrying the real marker (so is_agmsg_shim matches) + # plus a line shaped exactly like the one that used to get eval'd, except + # its "value" is a command substitution that -- if ever executed -- writes a + # sentinel file. Both status and a plain (non-force) install must leave + # that sentinel absent. + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME/.agents/bin" + local sentinel="$TEST_PROJECT/pwned" + cat > "$HOME/.agents/bin/codex" </ +# codex-shim-install.sh ...), via bash -c's argv0 trick, so "self" resolves +# to a real, chosen path rather than whatever sourcing this from a bats +# helper would otherwise produce. +_run_candidate_probe() { + local probe_home="$1" self_dir="$2" + run bash -c " + HOME='$probe_home' + source '$TYPES/codex/codex-shim-install.sh' >/dev/null 2>&1 + agmsg_install_candidates + echo '[end]' + if agmsg_only_one_install; then echo only_one=true; else echo only_one=false; fi + " "$self_dir/codex-shim-install.sh" +} + +_candidate_lines() { + echo "$output" | awk '/\[end\]/{exit}{print}' +} + +@test "agmsg_only_one_install: fresh install, self's own marker not written yet, no one else present" { + # The exact ordering gap review found: self is a real install about to + # register itself, but hasn't yet -- agmsg_install_candidates must not be + # the only thing that decides this; self must count even while invisible + # on disk. + export HOME="$TEST_PROJECT/home" + local self_dir="$HOME/.agents/skills/agmsg/scripts/drivers/types/codex" + # The skill directory tree (and this codex driver subdir within it) is + # laid down by install.sh well before the shim step -- only the .agmsg + # marker in the skill root is deferred. Create the tree, but not the + # marker, to match that ordering exactly. + mkdir -p "$self_dir" + + _run_candidate_probe "$HOME" "$self_dir" + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "only_one=true" + # nothing was listed before [end] -- an empty candidate set (self isn't on + # disk yet either). awk (not a sed "1,/re/" range) because that range + # never closes on line 1 itself, which is exactly this case ([end] IS + # line 1) -- it would silently keep reading past it instead. + [ -z "$(_candidate_lines)" ] +} + +@test "agmsg_only_one_install: fresh install, self's marker not written yet, but ONE other install already exists" { + # Same ordering gap as above, but this time there really is someone else + # -- pins that the allowance is specifically about self being invisible, + # not about being lenient whenever the total looks low. + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME/.agents/skills/agmsg" + touch "$HOME/.agents/skills/agmsg/.agmsg" + local self_dir="$HOME/.agents/skills/agmsg-dfr/scripts/drivers/types/codex" + mkdir -p "$self_dir" + + _run_candidate_probe "$HOME" "$self_dir" + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "only_one=false" + local candidate_lines; candidate_lines="$(_candidate_lines | grep -c .)" + [ "$candidate_lines" -eq 1 ] +} + +@test "agmsg_only_one_install: self already registered, name containing a space, no one else present" { + export HOME="$TEST_PROJECT/home" + local self_dir="$HOME/.agents/skills/agmsg dev/scripts/drivers/types/codex" + mkdir -p "$self_dir" + touch "$HOME/.agents/skills/agmsg dev/.agmsg" + + _run_candidate_probe "$HOME" "$self_dir" + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "only_one=true" + # the whole name (including the embedded space) survives as one line, not + # two, and is recognized as self (not an "other") -- agmsg_only_one_install + # counts lines that don't match self's own skill root, so a name that got + # word-split, or failed to compare equal to itself, would silently miscount. + printf '%s' "$output" | grep -qF "$HOME/.agents/skills/agmsg dev" + local candidate_lines; candidate_lines="$(_candidate_lines | grep -c .)" + [ "$candidate_lines" -eq 1 ] +} + +@test "agmsg_only_one_install: self plus one other, unmarked sibling not counted" { + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME/.agents/skills/one" "$HOME/.agents/skills/two" + touch "$HOME/.agents/skills/one/.agmsg" "$HOME/.agents/skills/two/.agmsg" + # an unmarked sibling directory must not be counted as a candidate + mkdir -p "$HOME/.agents/skills/not-agmsg" + local self_dir="$HOME/.agents/skills/one/scripts/drivers/types/codex" + mkdir -p "$self_dir" + + _run_candidate_probe "$HOME" "$self_dir" + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "only_one=false" + local candidate_lines; candidate_lines="$(_candidate_lines | grep -c .)" + [ "$candidate_lines" -eq 2 ] + refute grep -qF "/not-agmsg" <(printf '%s' "$output") +} diff --git a/tests/test_install.bats b/tests/test_install.bats index aa821f5db..ee20b31f9 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -664,6 +664,157 @@ PY [ ! -e "$FAKE_HOME/.agents/bin/codex" ] } +# #553: a second install under a different --cmd name used to silently +# rewrite ~/.agents/bin/codex to point at itself, so every Codex launch on the +# machine (through the shim) started dispatching into the second install's +# drivers/storage instead of the production one -- with no warning, printed +# as if it were a routine "refreshed" no-op. + +@test "install: a second, differently-named install does NOT clobber the first's Codex shim (#553)" { + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex + HOME="$FAKE_HOME" bash "$SK/scripts/drivers/types/codex/codex-shim-install.sh" install >/dev/null + local shim="$FAKE_HOME/.agents/bin/codex" + [ -f "$shim" ] + # Positive control: pin exactly which install owns it before touching + # anything else, byte for byte -- if this does not already say "agmsg", + # the rest of the test proves nothing. + local before; before="$(grep AGMSG_CODEX_SHIM_SCRIPT_DIR "$shim")" + printf '%s' "$before" | grep -qF "/skills/agmsg/" + + local sk2="$FAKE_HOME/.agents/skills/agmsg-dfr" + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --agent-type codex + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "owned by a different install" + + # The shim's bytes must be completely unchanged, not just "still valid" -- + # comparing the whole recorded line rather than only the owning dir catches + # a partial/malformed rewrite too. + local after; after="$(grep AGMSG_CODEX_SHIM_SCRIPT_DIR "$shim")" + [ "$before" = "$after" ] + [ -d "$sk2" ] # the second install itself still succeeded +} + +@test "install: --update --cmd can reclaim a Codex shim owned by a different install (#553)" { + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex + HOME="$FAKE_HOME" bash "$SK/scripts/drivers/types/codex/codex-shim-install.sh" install >/dev/null + local shim="$FAKE_HOME/.agents/bin/codex" + + local sk2="$FAKE_HOME/.agents/skills/agmsg-dfr" + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --agent-type codex >/dev/null + grep -q "/skills/agmsg/" "$shim" # still the first install's, per the test above + + # --update --cmd names a specific, already-registered install explicitly -- + # that explicit targeting is the documented recovery path, so it is allowed + # to reclaim the shim rather than being blocked like the fresh install above. + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --update + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "refreshed Codex monitor shim" + grep -q "/skills/agmsg-dfr/" "$shim" +} + +@test "install: bare --update (no --cmd) does NOT force-steal a Codex shim owned by a different install (#553)" { + # Unlike --update --cmd , a bare --update resolves its target by + # scanning for an existing install rather than the caller naming one --- and + # on this base (#599's fail-closed fix, PR #659, is not yet merged here), + # that resolution does not even fail closed when more than one install is + # present. Forcing the shim reclaim unconditionally for bare --update would + # let whichever install a glob happens to land on steal the shim from + # another one the caller never named at all (review finding). This pins + # that a shim already owned by a DIFFERENT install survives a bare --update + # of the install that does NOT own it. + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex + HOME="$FAKE_HOME" bash "$SK/scripts/drivers/types/codex/codex-shim-install.sh" install >/dev/null + local shim="$FAKE_HOME/.agents/bin/codex" + local before; before="$(grep AGMSG_CODEX_SHIM_SCRIPT_DIR "$shim")" + printf '%s' "$before" | grep -qF "/skills/agmsg/" + + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --agent-type codex >/dev/null + grep -q "/skills/agmsg/" "$shim" # still the first install's, per the earlier tests + + # Bare --update, no --cmd: this base's ambiguous-candidate handling means + # which of the two real installs it lands on isn't the point of this test + # (that's #599 / #659's concern) -- what matters here is that whichever one + # it is, it must not walk away with a shim it was never explicitly told to + # claim. + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --update + [ "$status" -eq 0 ] + local after; after="$(grep AGMSG_CODEX_SHIM_SCRIPT_DIR "$shim")" + [ "$before" = "$after" ] +} + +@test "install: bare --update migrates this machine's own pre-#553 (owner-unknown) Codex shim (#553)" { + # The two fixes above -- fail closed on an owner-unknown shim, and bare + # --update no longer forcing -- are each correct alone but combined to + # block the single-install upgrade they were never meant to touch: nearly + # every real machine's shim predates ownership tracking, has no owner + # comment, and a routine `install.sh --update` with no --cmd (the normal + # way a single-install user upgrades) must still be able to refresh it + # (review finding). Provably only one agmsg install existing at all is what + # makes that safe without needing --cmd or --force: there is no other + # install the shim could actually belong to. + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex + local shim="$FAKE_HOME/.agents/bin/codex" + mkdir -p "$FAKE_HOME/.agents/bin" + # A legacy shim: real marker, but written before this PR added the owner + # comment -- exactly what every pre-existing production shim looks like. + cat > "$shim" <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +# Optional Codex entrypoint shim for agmsg monitor mode. +# Generated by agmsg. Dispatches to the installed skill script. +export AGMSG_CODEX_SHIM_WRAPPER=1 +export AGMSG_CODEX_SHIM_SCRIPT_DIR=/some/stale/pre-move/path +exec /some/stale/pre-move/path/codex-shim.sh "$@" +EOF + chmod +x "$shim" + refute grep -q "agmsg-shim-owner" "$shim" + + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --update + [ "$status" -eq 0 ] + printf '%s' "$output" | grep -qF "refreshed Codex monitor shim" + grep -q "agmsg-shim-owner" "$shim" + grep -q "/skills/agmsg/scripts/drivers/types/codex" "$shim" + refute grep -q "/some/stale/pre-move/path" "$shim" +} + +@test "install: a second, differently-named FRESH install does NOT silently claim a pre-existing legacy shim (#553 review)" { + # Review finding: install.sh checks/refreshes the Codex shim (~line 436) + # BEFORE it touches this install's own .agmsg marker (~line 452). So when a + # second, differently-named install's fresh `install.sh --cmd` run reaches + # the shim step, agmsg_only_one_install sees only the FIRST install's + # marker on disk -- its own marker does not exist yet -- and (wrongly) + # concludes only one install exists anywhere, which is exactly the + # condition meant to let ONLY a genuinely sole install claim an + # owner-unknown legacy shim without --force. This pins that a second, + # differently-named install must not benefit from that allowance just + # because its own marker hasn't been written yet. + HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg --agent-type codex + local shim="$FAKE_HOME/.agents/bin/codex" + mkdir -p "$FAKE_HOME/.agents/bin" + # A legacy shim: real marker, but no owner comment -- same shape as any + # shim written before this PR, and the same fixture the bare-`--update` + # migration test above uses. + cat > "$shim" <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +# Optional Codex entrypoint shim for agmsg monitor mode. +# Generated by agmsg. Dispatches to the installed skill script. +export AGMSG_CODEX_SHIM_WRAPPER=1 +export AGMSG_CODEX_SHIM_SCRIPT_DIR=/some/stale/pre-move/path +exec /some/stale/pre-move/path/codex-shim.sh "$@" +EOF + chmod +x "$shim" + local before; before="$(cat "$shim")" + + # A second, DIFFERENT, freshly-created install -- not --update, so this + # install's own .agmsg marker genuinely does not exist until after the + # shim step runs. + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --cmd agmsg-dfr --agent-type codex + [ "$status" -eq 0 ] + + [ "$(cat "$shim")" = "$before" ] # byte-for-byte unchanged, not silently claimed +} + # --- grok-build skill (~/.grok/skills//SKILL.md) --- @test "install: drops a Grok Build SKILL.md when ~/.grok exists" {