From 44096e01e9da94fd8c2a057997bc1e55f4123605 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 05:19:10 -0700 Subject: [PATCH 1/7] fix(install): refuse to clobber a Codex shim owned by a different install (#553) install.sh --cmd unconditionally overwrote the shared ~/.agents/bin/codex shim, repointing every Codex launch on the machine into the new install's drivers/storage with no warning -- printed as a routine "refreshed" no-op. Recurred twice with real installs affected. codex-shim-install.sh now records and checks ownership: a new shim_owner_script_dir reads back which skill dir a shim was generated for, and `install` refuses to repoint a shim owned by a DIFFERENT install unless AGMSG_CODEX_SHIM_FORCE=1 is set, naming the current owner either way. `status` gains a second "owner:" line for the same reason. install.sh's two call sites split by how much authority they carry: --update explicitly names (or resolves) one specific, already-registered install, so it forces the reclaim -- the documented recovery path. A fresh --cmd install gives no such signal and does not force; it now surfaces (rather than silently swallows) a refusal, naming the AGMSG_CODEX_SHIM_FORCE incantation to claim the shim explicitly if that's actually wanted. Also fixes a real, measured regression the ownership check's own status line would otherwise have introduced: install.sh piped `status` straight into `grep -q '^installed:'` under `pipefail`. A second output line let `grep -q` close the pipe (it stops reading the instant it matches) while `status` was still writing that second line, so the write's SIGPIPE flipped the whole pipeline's exit status even though grep DID match. Fixed by capturing status into a variable before grepping it in both call sites. Positive control performed by hand before writing the fix: installed a production copy, then a second, differently-named copy, and confirmed byte-for-byte that the shim's recorded owner switched to the second install. Both new bats tests reproduce this same RED against the unfixed code (confirmed via git stash) before passing GREEN with the fix. --- install.sh | 43 +++++++-- .../drivers/types/codex/codex-shim-install.sh | 46 +++++++++ tests/test_codex_shim.bats | 93 +++++++++++++++++++ tests/test_install.bats | 48 ++++++++++ 4 files changed, 224 insertions(+), 6 deletions(-) diff --git a/install.sh b/install.sh index ee559ea89..69e5ddf18 100755 --- a/install.sh +++ b/install.sh @@ -320,9 +320,29 @@ 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: --update names (explicitly via --cmd, or by resolving the single + # existing install) exactly the skill dir the caller means to update, which + # is the documented recovery path for #553 (a different install's --cmd + # having clobbered the shim). That explicit targeting is what makes + # reclaiming the shim here safe — a FRESH install below gets no such signal + # and does not force. + # 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 \ + 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 + AGMSG_CODEX_SHIM_INSTALL_QUIET=1 AGMSG_CODEX_SHIM_FORCE=1 "$CODEX_SHIM" install >/dev/null 2>&1 \ && echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" fi install_windows_helpers @@ -390,11 +410,22 @@ 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 + if AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" install >/dev/null 2>&1; then + echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" + else + echo " ! Codex monitor shim (~/.agents/bin/codex) is owned by a different install; left untouched." + echo " Run 'AGMSG_CODEX_SHIM_FORCE=1 $CODEX_SHIM install' to claim it for this install instead." + 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..0220427d2 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -24,6 +24,24 @@ is_agmsg_shim() { [ -f "$TARGET" ] && grep -q "Optional Codex entrypoint shim for agmsg monitor mode" "$TARGET" 2>/dev/null } +# Prints the skill script dir baked into the currently-installed shim (empty +# if there is none, or it is not an agmsg shim). 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. +# +# Extracts and evals only the single `export ...=` line (never sources the +# whole file — the file's last line execs into codex-shim.sh, and sourcing it +# would actually launch that, not just read a variable back out). shell_quote +# writes the value with bash's own %q, which eval reverses correctly; is_agmsg_shim +# having already matched means this is a file agmsg generated, not arbitrary input. +shim_owner_script_dir() { + is_agmsg_shim || return 0 + local line + line="$(grep '^export AGMSG_CODEX_SHIM_SCRIPT_DIR=' "$TARGET" 2>/dev/null | head -1)" + [ -n "$line" ] || return 0 + ( eval "$line" 2>/dev/null; printf '%s' "${AGMSG_CODEX_SHIM_SCRIPT_DIR:-}" ) +} + shell_quote() { printf '%q' "$1" } @@ -48,6 +66,21 @@ 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. Refuse + # to hand it to a different skill dir without an explicit ask, naming which + # install currently owns it so the caller can decide (re-run with + # AGMSG_CODEX_SHIM_FORCE=1 to reclaim it, or leave it alone). + owner="$(shim_owner_script_dir)" + if [ -n "$owner" ] && [ "$owner" != "$SCRIPT_DIR" ] && [ "${AGMSG_CODEX_SHIM_FORCE:-}" != "1" ]; then + echo "codex-shim-install: $TARGET is owned by a different install:" >&2 + echo " $owner" >&2 + echo "codex-shim-install: refusing to repoint it at $SCRIPT_DIR" >&2 + echo "codex-shim-install: re-run with AGMSG_CODEX_SHIM_FORCE=1 to claim it for this install instead" >&2 + exit 1 + fi { echo "#!/usr/bin/env bash" echo "set -euo pipefail" @@ -85,6 +118,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 [ -n "$owner" ]; then + if [ "$owner" = "$SCRIPT_DIR" ]; then + echo "owner: this install ($owner)" + else + echo "owner: a different install ($owner)" + fi + fi else echo "not installed: $TARGET" fi diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 09ebbce02..24f533d6e 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -248,3 +248,96 @@ 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 ] + [[ "$output" == *"owned by a different install"* ]] + [[ "$output" == *"$TYPES/codex"* ]] # names the actual owner, not just "someone else" + [[ "$output" == *"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 ] + [[ "$output" == *"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 ] + [[ "$output" == *"refusing to overwrite existing"* ]] + [[ "$output" != *"owned by a different install"* ]] + [ "$(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 +} diff --git a/tests/test_install.bats b/tests/test_install.bats index aa821f5db..e40d0bd03 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -664,6 +664,54 @@ 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")" + [[ "$before" == *"/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 ] + [[ "$output" == *"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 ] + [[ "$output" == *"refreshed Codex monitor shim"* ]] + grep -q "/skills/agmsg-dfr/" "$shim" +} + # --- grok-build skill (~/.grok/skills//SKILL.md) --- @test "install: drops a Grok Build SKILL.md when ~/.grok exists" { From b083a053c5e034cf2010040d1f2b5ec731e81fb6 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 05:29:59 -0700 Subject: [PATCH 2/7] fix(install): stop evaling shim-derived content; state the consequence of forcing (review) Two issues raised in review, both against codex-shim-install.sh: 1. shim_owner_script_dir extracted the owner via `eval` on a line read back from the shim file. is_agmsg_shim's authenticity check is a grep for one marker string -- it proves nothing about the rest of a file that lives at a local, single-user, normally-writable-by-that-user path and could have been hand-edited afterward. A crafted file carrying the marker plus a line shaped like the real export (e.g. with a command substitution as its "value") turned a read-only `status` call into arbitrary code execution (raised independently by two reviewers). Fixed by dropping eval entirely. `install` now writes a dedicated, never-executed comment line (`# agmsg-shim-owner: `) alongside the real, executable export line the shim needs at runtime, and shim_owner_script_dir reads only that comment via plain text extraction. No interpretation of file content as code, regardless of what the file contains. 2. The refusal message named the AGMSG_CODEX_SHIM_FORCE=1 escape hatch but not what using it actually does. Confirmed against tonight's own recurring pattern (a printed remedy that doesn't say what state it leaves you in isn't a remedy yet): now states explicitly that forcing repoints every Codex launch through the shim from the current owner to this install. install.sh's fresh-install call site no longer discards the shim's own stderr on refusal, so this full message actually reaches the user during a normal install instead of being silently swallowed. Forced reproduction for the eval removal: reverted shim_owner_script_dir to the eval-based version, confirmed the new "tampered shim cannot execute code" test fails (RED) -- the sentinel file it plants gets created via the tampered export line -- then restored the fix and confirmed the sentinel is never created (GREEN), for both `status` and `install`. --- install.sh | 10 +++--- .../drivers/types/codex/codex-shim-install.sh | 29 +++++++++------ tests/test_codex_shim.bats | 36 +++++++++++++++++++ 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/install.sh b/install.sh index 69e5ddf18..fead4d828 100755 --- a/install.sh +++ b/install.sh @@ -420,11 +420,13 @@ CODEX_SHIM="$SKILL_DIR/scripts/drivers/types/codex/codex-shim-install.sh" 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 - if AGMSG_CODEX_SHIM_INSTALL_QUIET=1 "$CODEX_SHIM" install >/dev/null 2>&1; 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)" - else - echo " ! Codex monitor shim (~/.agents/bin/codex) is owned by a different install; left untouched." - echo " Run 'AGMSG_CODEX_SHIM_FORCE=1 $CODEX_SHIM install' to claim it for this install instead." 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 0220427d2..c2fc8b18a 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -29,17 +29,19 @@ is_agmsg_shim() { # "owns" the shim: the one whose codex-shim.sh it execs into and whose # storage/drivers every Codex launch through it will resolve. # -# Extracts and evals only the single `export ...=` line (never sources the -# whole file — the file's last line execs into codex-shim.sh, and sourcing it -# would actually launch that, not just read a variable back out). shell_quote -# writes the value with bash's own %q, which eval reverses correctly; is_agmsg_shim -# having already matched means this is a file agmsg generated, not arbitrary input. +# Reads a DEDICATED comment line (`# agmsg-shim-owner: `, 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. shim_owner_script_dir() { is_agmsg_shim || return 0 - local line - line="$(grep '^export AGMSG_CODEX_SHIM_SCRIPT_DIR=' "$TARGET" 2>/dev/null | head -1)" - [ -n "$line" ] || return 0 - ( eval "$line" 2>/dev/null; printf '%s' "${AGMSG_CODEX_SHIM_SCRIPT_DIR:-}" ) + sed -n 's/^# agmsg-shim-owner: //p' "$TARGET" 2>/dev/null | head -1 } shell_quote() { @@ -78,7 +80,9 @@ EOF echo "codex-shim-install: $TARGET is owned by a different install:" >&2 echo " $owner" >&2 echo "codex-shim-install: refusing to repoint it at $SCRIPT_DIR" >&2 - echo "codex-shim-install: re-run with AGMSG_CODEX_SHIM_FORCE=1 to claim it for this install instead" >&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" >&2 + echo "codex-shim-install: $SCRIPT_DIR instead of $owner" >&2 exit 1 fi { @@ -87,6 +91,11 @@ EOF 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. Kept separate from the + # export line below, which is real, executable, runtime-required state + # for codex-shim.sh, quoted for shell re-use rather than safe display. + echo "# agmsg-shim-owner: $SCRIPT_DIR" echo "" echo "export AGMSG_CODEX_SHIM_WRAPPER=1" echo "export AGMSG_CODEX_SHIM_SCRIPT_DIR=$(shell_quote "$SCRIPT_DIR")" diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 24f533d6e..5991bd3a8 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -341,3 +341,39 @@ _second_codex_dir() { 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" < Date: Mon, 10 Aug 2026 05:42:15 -0700 Subject: [PATCH 3/7] fix(install): fail closed on legacy/unowned shims; scope --update forcing (review) Two issues raised in review, both against the ownership guard added earlier: 1. An existing agmsg shim written before this PR shipped has no `# agmsg-shim-owner:` line at all. shim_owner_script_dir returns empty for it, and the install guard required owner to be non-empty before refusing -- so a shim with unknown, unrecorded ownership was treated as "unowned, safe to take" and silently overwritten by any new, differently-named install. That is #553's own bug, recurring at exactly the migration moment it matters most: the first time a second install runs an installer carrying this fix against a production shim that predates it. Fixed by treating "is an agmsg shim (marker matches) but records no owner" as "legacy owner, unknown" rather than "unowned" -- it now fails closed the same as a foreign-owned shim, distinguishing the two only in the message shown (named owner vs. "predates ownership tracking"). AGMSG_CODEX_SHIM_FORCE=1 still claims it either way. Also, non-blocking but folded in: the owner comment now stores the %q-quoted path (matching the executable export line's own quoting) rather than the raw path, and every reader compares/displays the quoted form -- a path containing a literal newline could otherwise have forged a second, fake comment line of its own. 2. install.sh's --update block forced the shim reclaim unconditionally, including for a bare `--update` with no `--cmd`. That resolves its target by scanning for a single existing install rather than the caller naming one, and on this base (integration/remote) that scan does not fail closed when more than one install is present -- the fail-closed fix for that (#599) is PR #659, still pending against main and not merged here. Forcing unconditionally meant whichever install a glob happened to resolve to could steal the shim from another install the caller never named at all, compounding #599 with a #553-shaped consequence. Fixed by capturing whether --cmd was passed explicitly (before CMD_NAME gets defaulted or resolved to anything else) and forcing only then. Bare `--update` still refreshes a shim this same install already owns (the common single-install case, unaffected), but no longer reaches past one it doesn't. Forced reproduction for both: reverted each fix in turn, confirmed the matching test fails (RED) against the reverted code -- a crafted legacy shim gets silently claimed in the first case, an unrelated install's shim gets silently stolen by an ambiguously-resolved bare --update in the second -- then restored the fix and confirmed both pass (GREEN). --- install.sh | 33 +++++-- .../drivers/types/codex/codex-shim-install.sh | 92 ++++++++++++------- tests/test_codex_shim.bats | 16 +++- tests/test_install.bats | 30 ++++++ 4 files changed, 124 insertions(+), 47 deletions(-) diff --git a/install.sh b/install.sh index fead4d828..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 @@ -321,12 +326,21 @@ if [ "$UPDATE_ONLY" = true ]; then # 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: --update names (explicitly via --cmd, or by resolving the single - # existing install) exactly the skill dir the caller means to update, which - # is the documented recovery path for #553 (a different install's --cmd - # having clobbered the shim). That explicit targeting is what makes - # reclaiming the shim here safe — a FRESH install below gets no such signal - # and does not force. + # 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, @@ -342,8 +356,11 @@ if [ "$UPDATE_ONLY" = true ]; then 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 - AGMSG_CODEX_SHIM_INSTALL_QUIET=1 AGMSG_CODEX_SHIM_FORCE=1 "$CODEX_SHIM" install >/dev/null 2>&1 \ - && echo " + refreshed Codex monitor shim (~/.agents/bin/codex)" + 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)" diff --git a/scripts/drivers/types/codex/codex-shim-install.sh b/scripts/drivers/types/codex/codex-shim-install.sh index c2fc8b18a..da3f57a91 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -24,30 +24,39 @@ is_agmsg_shim() { [ -f "$TARGET" ] && grep -q "Optional Codex entrypoint shim for agmsg monitor mode" "$TARGET" 2>/dev/null } -# Prints the skill script dir baked into the currently-installed shim (empty -# if there is none, or it is not an agmsg shim). This is the install that +shell_quote() { + printf '%q' "$1" +} + +# 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: `, 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. +# 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 } -shell_quote() { - printf '%q' "$1" -} - cmd="${1:-function}" case "$cmd" in -h|--help) @@ -71,18 +80,30 @@ EOF # 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. Refuse - # to hand it to a different skill dir without an explicit ask, naming which - # install currently owns it so the caller can decide (re-run with - # AGMSG_CODEX_SHIM_FORCE=1 to reclaim it, or leave it alone). + # 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)" - if [ -n "$owner" ] && [ "$owner" != "$SCRIPT_DIR" ] && [ "${AGMSG_CODEX_SHIM_FORCE:-}" != "1" ]; then - echo "codex-shim-install: $TARGET is owned by a different install:" >&2 - echo " $owner" >&2 + if [ -e "$TARGET" ] && [ "$owner" != "$self_owner" ] && [ "${AGMSG_CODEX_SHIM_FORCE:-}" != "1" ]; 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" >&2 - echo "codex-shim-install: $SCRIPT_DIR instead of $owner" >&2 + echo "codex-shim-install: every Codex launch that goes through the shim will then dispatch into $SCRIPT_DIR" >&2 exit 1 fi { @@ -92,10 +113,11 @@ EOF 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. Kept separate from the - # export line below, which is real, executable, runtime-required state - # for codex-shim.sh, quoted for shell re-use rather than safe display. - echo "# agmsg-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")" @@ -133,12 +155,12 @@ EOF # 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 [ -n "$owner" ]; then - if [ "$owner" = "$SCRIPT_DIR" ]; then - echo "owner: this install ($owner)" - else - echo "owner: a different install ($owner)" - fi + 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" diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 5991bd3a8..8053d1da8 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -366,14 +366,22 @@ EOF run bash "$TYPES/codex/codex-shim-install.sh" status [ "$status" -eq 0 ] + [[ "$output" == *"owner: unknown"* ]] [ ! -e "$sentinel" ] # No `# agmsg-shim-owner:` line at all (this crafted file predates it, same - # as any real shim written before this PR) reads as owner="" -- unowned, not - # foreign -- so install proceeds and overwrites it, same as it always could. - # The only property under test here is that doing so never executes - # anything the tampered content contained. + # as any real shim written before this PR) reads as owner unknown, which + # fails closed the same as a foreign owner would (#553 review: "unowned" and + # "owner not recorded" are not the same claim, and treating them the same + # would silently repeat #553's own bug against every pre-existing shim on + # first contact with a second, differently-named install). The property + # under test either way: nothing the tampered content contains ever runs. run bash "$TYPES/codex/codex-shim-install.sh" install + [ "$status" -ne 0 ] + [[ "$output" == *"before ownership tracking"* ]] + [ ! -e "$sentinel" ] + + AGMSG_CODEX_SHIM_FORCE=1 run bash "$TYPES/codex/codex-shim-install.sh" install [ "$status" -eq 0 ] [ ! -e "$sentinel" ] } diff --git a/tests/test_install.bats b/tests/test_install.bats index e40d0bd03..c4fa4cefa 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -712,6 +712,36 @@ PY 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")" + [[ "$before" == *"/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" ] +} + # --- grok-build skill (~/.grok/skills//SKILL.md) --- @test "install: drops a Grok Build SKILL.md when ~/.grok exists" { From 0dbefaaf84d33d6fac402dd8ee9f6b64db7529e9 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 05:51:55 -0700 Subject: [PATCH 4/7] fix(install): let a provably-sole install migrate its own legacy shim (review) The previous two fixes -- failing closed on an owner-unknown (legacy, pre- this-feature) shim, and no longer forcing bare `--update` -- are each correct alone but combined to block the single-install upgrade path they were never meant to touch. Nearly every real machine's existing shim predates ownership tracking and has no owner comment; a routine `install.sh --update` with no `--cmd` (how a single-install user normally upgrades) could no longer refresh it at all, leaving a stale shim (possibly still pointing at a pre-move path) in place indefinitely. Adds agmsg_only_one_install: true iff exactly one `.agmsg`-marked directory exists under ~/.agents/skills, derived directly from this machine's actual state rather than assumed from whether #599's fail-closed multi-install handling (PR #659) is merged wherever this runs -- which it may not be. An owner-unknown shim is now claimable without --cmd/--force specifically when this holds: with only one agmsg install anywhere, nothing else could have written the shim, so there is no one to take it from. With two or more installs present the check returns false and the existing fail-closed behavior is unchanged. Forced reproduction: reverted the allowance, confirmed the new "bare --update migrates this machine's own pre-#553 shim" test fails (RED) -- the legacy shim is left stale, still pointing at its old path -- then restored the fix and confirmed it migrates to the new owner-tracked format (GREEN). --- .../drivers/types/codex/codex-shim-install.sh | 38 ++++++++++++++++++- tests/test_install.bats | 35 +++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/scripts/drivers/types/codex/codex-shim-install.sh b/scripts/drivers/types/codex/codex-shim-install.sh index da3f57a91..b2606e047 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -28,6 +28,31 @@ shell_quote() { printf '%q' "$1" } +# True iff exactly one agmsg install (a `.agmsg`-marked directory) exists +# anywhere under ~/.agents/skills. Derives the candidate 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). +# +# 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. +agmsg_only_one_install() { + local skills_dir count=0 d + skills_dir="$(dirname "$AGENTS_BIN")/skills" + [ -d "$skills_dir" ] || return 1 + for d in "$skills_dir"/*/; do + [ -f "${d}.agmsg" ] || continue + count=$((count + 1)) + [ "$count" -gt 1 ] && return 1 + done + [ "$count" -eq 1 ] +} + # 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 @@ -93,7 +118,18 @@ EOF # recording THIS install's own SCRIPT_DIR skips the guard. self_owner="$(shell_quote "$SCRIPT_DIR")" owner="$(shim_owner_script_dir)" - if [ -e "$TARGET" ] && [ "$owner" != "$self_owner" ] && [ "${AGMSG_CODEX_SHIM_FORCE:-}" != "1" ]; then + # 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 diff --git a/tests/test_install.bats b/tests/test_install.bats index c4fa4cefa..351b7dee6 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -742,6 +742,41 @@ PY [ "$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" + ! grep -q "agmsg-shim-owner" "$shim" + + run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --update + [ "$status" -eq 0 ] + [[ "$output" == *"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" +} + # --- grok-build skill (~/.grok/skills//SKILL.md) --- @test "install: drops a Grok Build SKILL.md when ~/.grok exists" { From 5fa3f9413d0a6216052121e07badd059efb1b626 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 07:09:57 -0700 Subject: [PATCH 5/7] refactor(install): split candidate listing out of agmsg_only_one_install (review) Non-blocking review note: agmsg_only_one_install's ~/.agents/skills scan answers the same question #659 (still open against main, unmerged) will independently need for its own bare-`--update` candidate enumeration -- the third time tonight two implementations have answered the same question separately (after #722 -> #733, #741's config validation). Splits the scan into agmsg_install_candidates (lists candidate install dirs, one per line) with agmsg_only_one_install now just counting its output. This doesn't unify the two implementations -- #659 doesn't exist on this base to point at yet -- but gives it something to call instead of re-scanning ~/.agents/skills a second time once it lands on a shared base with this file. Behavior unchanged; tests unchanged and still 73/73. --- .../drivers/types/codex/codex-shim-install.sh | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/scripts/drivers/types/codex/codex-shim-install.sh b/scripts/drivers/types/codex/codex-shim-install.sh index b2606e047..5d035a814 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -28,12 +28,30 @@ shell_quote() { printf '%q' "$1" } -# True iff exactly one agmsg install (a `.agmsg`-marked directory) exists -# anywhere under ~/.agents/skills. Derives the candidate 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). +# 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 agmsg_install_candidates lists exactly one directory. # # What this buys: a legacy shim (agmsg's, but predating ownership tracking, # #553) has no recorded owner to compare against -- but if this is @@ -42,14 +60,8 @@ shell_quote() { # more installs present, this returns false and the caller falls back to # fail-closed, same as an unrecorded owner always has since #553. agmsg_only_one_install() { - local skills_dir count=0 d - skills_dir="$(dirname "$AGENTS_BIN")/skills" - [ -d "$skills_dir" ] || return 1 - for d in "$skills_dir"/*/; do - [ -f "${d}.agmsg" ] || continue - count=$((count + 1)) - [ "$count" -gt 1 ] && return 1 - done + local count + count="$(agmsg_install_candidates | wc -l | tr -d ' ')" [ "$count" -eq 1 ] } From bb4e4d06438140991842656a20b2c591385c2ec3 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 07:20:34 -0700 Subject: [PATCH 6/7] test(install): cover agmsg_install_candidates/agmsg_only_one_install directly (review) Review: "behavior unchanged" after splitting agmsg_only_one_install into a listing half (agmsg_install_candidates) and a counting half is a claim, not something 73 pre-existing, unrelated tests already prove -- none of them exercise the split's own boundary. In particular, agmsg_only_one_ install now counts lines from agmsg_install_candidates' output, so a candidate name containing a space is a real risk this split introduced: if the listing ever word-split instead of emitting one line per directory, a single real install would silently miscount as more than one and a routine upgrade would wrongly fail closed. Adds three direct unit tests (source the script, call the functions): zero installs, exactly one install with a space in its name, and two installs alongside an unmarked sibling directory that must not be counted. All three source codex-shim-install.sh in a subshell and read its output through a '[end]' marker rather than parsing raw text, which uncovered its own bug while writing these: a GNU sed '1,/re/' range does not close on line 1 even when line 1 matches, so the zero-installs case (where '[end]' IS line 1) silently kept reading past it. Replaced with an awk '/end/{exit}{print}' extraction, which is correct regardless of where the marker falls. 76/76 pass (test_codex_shim.bats + test_install.bats). --- tests/test_codex_shim.bats | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 8053d1da8..692d9b0db 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -385,3 +385,67 @@ EOF [ "$status" -eq 0 ] [ ! -e "$sentinel" ] } + +# --- agmsg_install_candidates / agmsg_only_one_install (#553 review): these +# were split from one function into two (list, then count) so a future #659 +# could call the listing half instead of re-scanning ~/.agents/skills itself. +# The split changes what "counting" means -- it now depends on the listing +# producing exactly one line per candidate, including when a candidate name +# contains a space -- so it needs its own direct coverage, not just reliance +# on the pre-existing install.sh-level tests continuing to pass. +_run_candidate_probe() { + local probe_home="$1" + 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 + " +} + +@test "agmsg_install_candidates/agmsg_only_one_install: no installs at all" { + export HOME="$TEST_PROJECT/home" + mkdir -p "$HOME/.agents/skills" + + _run_candidate_probe "$HOME" + [ "$status" -eq 0 ] + [[ "$output" == *"only_one=false"* ]] + # nothing was listed before [end] -- an empty candidate set. awk (not a sed + # "1,/re/" range) because that range never closes on line 1 itself, which + # is exactly the zero-candidates case here ([end] IS line 1) -- it would + # silently keep reading past it instead. + [[ "$(echo "$output" | awk '/\[end\]/{exit}{print}')" == "" ]] +} + +@test "agmsg_install_candidates/agmsg_only_one_install: exactly one install, name containing a space" { + export HOME="$TEST_PROJECT/home" + local dir="$HOME/.agents/skills/agmsg dev" + mkdir -p "$dir" + touch "$dir/.agmsg" + + _run_candidate_probe "$HOME" + [ "$status" -eq 0 ] + [[ "$output" == *"only_one=true"* ]] + # the whole name (including the embedded space) survives as one line, not + # two -- agmsg_only_one_install counts lines, so a name that got word-split + # would silently miscount even a single real install as more than one. + [[ "$output" == *"$dir"* ]] + local candidate_lines; candidate_lines="$(echo "$output" | awk '/\[end\]/{exit}{print}' | grep -c .)" + [ "$candidate_lines" -eq 1 ] +} + +@test "agmsg_install_candidates/agmsg_only_one_install: two installs present" { + 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" + + _run_candidate_probe "$HOME" + [ "$status" -eq 0 ] + [[ "$output" == *"only_one=false"* ]] + local candidate_lines; candidate_lines="$(echo "$output" | awk '/\[end\]/{exit}{print}' | grep -c .)" + [ "$candidate_lines" -eq 2 ] + [[ "$output" != *"/not-agmsg"* ]] +} From dfb3b7ad07c4a284d4244079ca63b5fd8a738b99 Mon Sep 17 00:00:00 2001 From: fujibee Date: Mon, 10 Aug 2026 07:46:22 -0700 Subject: [PATCH 7/7] fix(install): self must count as a candidate before its own marker exists (review) Review (independently verified against the actual code): agmsg_only_one_ install decided "only one install" by counting REAL .agmsg markers on disk, but install.sh's fresh --cmd path checks/refreshes the Codex shim before it touches this install's own marker. So during a fresh, second, differently-named install, this install's own marker genuinely does not exist yet at the moment the check runs -- the count then only sees the FIRST install's marker, concludes "only one install exists anywhere", and lets the second install silently claim a legacy (owner-unknown) shim without --force. That is #553's own bug, reappearing specifically for the single-legacy-shim case the previous round's fix was meant to keep working. Fixed by counting OTHER installs relative to self (SCRIPT_DIR's own skill root, recovered by stripping the fixed .../scripts/drivers/types/ codex suffix install.sh always lays this script out under) rather than a plain total. Self counts as a candidate whether or not its own marker is written yet -- but only when self's recovered skill root structurally sits under ~/.agents/skills; otherwise (e.g. running this script directly against a repo checkout, as several of this file's own tests do) self isn't trusted as an implicit member, and the check falls back to the previous exact-one-real-marker behavior. Forced reproduction: added "a second, differently-named FRESH install does NOT silently claim a pre-existing legacy shim" (test_install.bats) and confirmed it fails (RED) against the unfixed self_root-unaware version -- the second install's shim step reads the first install's lone marker, agmsg_only_one_install returns true, and the shim silently repoints. Restored the fix, confirmed GREEN. Also adds direct unit coverage for agmsg_only_one_install's boundary (test_codex_shim.bats): self invisible with zero/one other real install, self already registered with a space in its name, and an unmarked sibling correctly excluded -- each constructs a realistic $0 via bash -c's argv0 so SCRIPT_DIR resolves the same way it does in a real invocation. Also fixes the enforced-assertions gate this round's own additions pushed 18 over baseline (616): non-last `[[ ]]`/`! cmd` checks in bats tests can't fail the suite on macOS bash 3.2 (#670) -- converted to `grep -qF`/`refute` (both plain commands, always enforced) or left as `[ ]`, which was never flagged. No behavior change; back to exactly 616. 78/78 pass (test_codex_shim.bats + test_install.bats). --- .../drivers/types/codex/codex-shim-install.sh | 50 +++++++- tests/test_codex_shim.bats | 115 ++++++++++++------ tests/test_install.bats | 50 +++++++- 3 files changed, 167 insertions(+), 48 deletions(-) diff --git a/scripts/drivers/types/codex/codex-shim-install.sh b/scripts/drivers/types/codex/codex-shim-install.sh index 5d035a814..30d1ab1af 100755 --- a/scripts/drivers/types/codex/codex-shim-install.sh +++ b/scripts/drivers/types/codex/codex-shim-install.sh @@ -51,7 +51,8 @@ agmsg_install_candidates() { done } -# True iff agmsg_install_candidates lists exactly one directory. +# 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 @@ -59,10 +60,51 @@ agmsg_install_candidates() { # 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 count - count="$(agmsg_install_candidates | wc -l | tr -d ' ')" - [ "$count" -eq 1 ] + 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- diff --git a/tests/test_codex_shim.bats b/tests/test_codex_shim.bats index 692d9b0db..e3daad35c 100644 --- a/tests/test_codex_shim.bats +++ b/tests/test_codex_shim.bats @@ -271,9 +271,9 @@ _second_codex_dir() { local second_dir; second_dir="$(_second_codex_dir)" run bash "$second_dir/codex-shim-install.sh" install [ "$status" -ne 0 ] - [[ "$output" == *"owned by a different install"* ]] - [[ "$output" == *"$TYPES/codex"* ]] # names the actual owner, not just "someone else" - [[ "$output" == *"AGMSG_CODEX_SHIM_FORCE=1"* ]] + 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 } @@ -296,7 +296,7 @@ _second_codex_dir() { run bash "$TYPES/codex/codex-shim-install.sh" status [ "$status" -eq 0 ] - [[ "$output" == *"owner: this install ($TYPES/codex)"* ]] + 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 @@ -317,8 +317,8 @@ _second_codex_dir() { run bash "$TYPES/codex/codex-shim-install.sh" install [ "$status" -ne 0 ] - [[ "$output" == *"refusing to overwrite existing"* ]] - [[ "$output" != *"owned by a different install"* ]] + 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" ] } @@ -366,7 +366,7 @@ EOF run bash "$TYPES/codex/codex-shim-install.sh" status [ "$status" -eq 0 ] - [[ "$output" == *"owner: unknown"* ]] + printf '%s' "$output" | grep -qF "owner: unknown" [ ! -e "$sentinel" ] # No `# agmsg-shim-owner:` line at all (this crafted file predates it, same @@ -378,7 +378,7 @@ EOF # under test either way: nothing the tampered content contains ever runs. run bash "$TYPES/codex/codex-shim-install.sh" install [ "$status" -ne 0 ] - [[ "$output" == *"before ownership tracking"* ]] + printf '%s' "$output" | grep -qF "before ownership tracking" [ ! -e "$sentinel" ] AGMSG_CODEX_SHIM_FORCE=1 run bash "$TYPES/codex/codex-shim-install.sh" install @@ -389,63 +389,102 @@ EOF # --- agmsg_install_candidates / agmsg_only_one_install (#553 review): these # were split from one function into two (list, then count) so a future #659 # could call the listing half instead of re-scanning ~/.agents/skills itself. -# The split changes what "counting" means -- it now depends on the listing -# producing exactly one line per candidate, including when a candidate name -# contains a space -- so it needs its own direct coverage, not just reliance -# on the pre-existing install.sh-level tests continuing to pass. +# agmsg_only_one_install answers "zero OTHER installs besides me (SCRIPT_DIR)" +# rather than "exactly one candidate total" (second review round): install.sh +# checks/refreshes the shim BEFORE it touches this install's own .agmsg +# marker, so during a fresh install self's own marker genuinely isn't on disk +# yet -- a plain total-candidate count would then undercount and let a +# genuinely second install claim a legacy shim it was never told to take. +# These probes set $0 the same way a real invocation does (bash / +# 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" + 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" } -@test "agmsg_install_candidates/agmsg_only_one_install: no installs at all" { +_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" + 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" + _run_candidate_probe "$HOME" "$self_dir" [ "$status" -eq 0 ] - [[ "$output" == *"only_one=false"* ]] - # nothing was listed before [end] -- an empty candidate set. awk (not a sed - # "1,/re/" range) because that range never closes on line 1 itself, which - # is exactly the zero-candidates case here ([end] IS line 1) -- it would - # silently keep reading past it instead. - [[ "$(echo "$output" | awk '/\[end\]/{exit}{print}')" == "" ]] + printf '%s' "$output" | grep -qF "only_one=false" + local candidate_lines; candidate_lines="$(_candidate_lines | grep -c .)" + [ "$candidate_lines" -eq 1 ] } -@test "agmsg_install_candidates/agmsg_only_one_install: exactly one install, name containing a space" { +@test "agmsg_only_one_install: self already registered, name containing a space, no one else present" { export HOME="$TEST_PROJECT/home" - local dir="$HOME/.agents/skills/agmsg dev" - mkdir -p "$dir" - touch "$dir/.agmsg" + 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" + _run_candidate_probe "$HOME" "$self_dir" [ "$status" -eq 0 ] - [[ "$output" == *"only_one=true"* ]] + printf '%s' "$output" | grep -qF "only_one=true" # the whole name (including the embedded space) survives as one line, not - # two -- agmsg_only_one_install counts lines, so a name that got word-split - # would silently miscount even a single real install as more than one. - [[ "$output" == *"$dir"* ]] - local candidate_lines; candidate_lines="$(echo "$output" | awk '/\[end\]/{exit}{print}' | grep -c .)" + # 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_install_candidates/agmsg_only_one_install: two installs present" { +@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" + _run_candidate_probe "$HOME" "$self_dir" [ "$status" -eq 0 ] - [[ "$output" == *"only_one=false"* ]] - local candidate_lines; candidate_lines="$(echo "$output" | awk '/\[end\]/{exit}{print}' | grep -c .)" + printf '%s' "$output" | grep -qF "only_one=false" + local candidate_lines; candidate_lines="$(_candidate_lines | grep -c .)" [ "$candidate_lines" -eq 2 ] - [[ "$output" != *"/not-agmsg"* ]] + refute grep -qF "/not-agmsg" <(printf '%s' "$output") } diff --git a/tests/test_install.bats b/tests/test_install.bats index 351b7dee6..ee20b31f9 100644 --- a/tests/test_install.bats +++ b/tests/test_install.bats @@ -679,12 +679,12 @@ PY # 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")" - [[ "$before" == *"/skills/agmsg/"* ]] + 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 ] - [[ "$output" == *"owned by a different install"* ]] + 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 @@ -708,7 +708,7 @@ PY # 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 ] - [[ "$output" == *"refreshed Codex monitor shim"* ]] + printf '%s' "$output" | grep -qF "refreshed Codex monitor shim" grep -q "/skills/agmsg-dfr/" "$shim" } @@ -726,7 +726,7 @@ PY 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")" - [[ "$before" == *"/skills/agmsg/"* ]] + 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 @@ -767,16 +767,54 @@ export AGMSG_CODEX_SHIM_SCRIPT_DIR=/some/stale/pre-move/path exec /some/stale/pre-move/path/codex-shim.sh "$@" EOF chmod +x "$shim" - ! grep -q "agmsg-shim-owner" "$shim" + refute grep -q "agmsg-shim-owner" "$shim" run env HOME="$FAKE_HOME" bash "$REPO_ROOT/install.sh" --update [ "$status" -eq 0 ] - [[ "$output" == *"refreshed Codex monitor shim"* ]] + 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" {