fix(install): refuse to clobber a Codex shim owned by a different install (#553)

[fujibee]

Summary

install.sh --cmd <other-name> unconditionally overwrote the shared ~/.agents/bin/codex shim, repointing every Codex launch on the machine (through the shim) into the new install's drivers/storage — silently, printed as a routine "refreshed Codex monitor shim" no-op. Recurred twice with real installs affected (issue comment has the second occurrence's measured blast radius).

• codex-shim-install.sh now records and checks ownership: shim_owner_script_dir reads back which skill dir a shim was generated for. install refuses to repoint a shim owned by a different install (or one whose ownership isn't recorded at all — see the legacy-shim fix below) unless AGMSG_CODEX_SHIM_FORCE=1 is set, naming the current owner either way (never a bare "no"). status gains a second owner: line for the same reason.
• install.sh's two call sites split by how much authority they carry:
  • --update --cmd <name> explicitly names one specific, already-registered install — that's the documented recovery path (re-run it against the production name to repoint the shim back), so it forces the reclaim. (Bare --update with no --cmd does not force — see the scoping fix below.)
  • A fresh --cmd install gives no such signal and does not force. It now surfaces (rather than silently swallows) a refusal, naming the exact AGMSG_CODEX_SHIM_FORCE=1 ... install command to run if reclaiming is actually wanted.

Also fixed: a real regression the ownership check's own output would have introduced

install.sh piped codex-shim-install.sh status straight into grep -q '^installed:' under this script's set -o pipefail. Adding the second owner: line broke that: grep -q stops reading (and closes its end of the pipe) the instant it matches the first line, and status's own echo of the second line then hits a reader that's already gone — SIGPIPE, a nonzero exit for that pipeline stage — which pipefail reports as the whole pipeline failing even though grep did match. Measured directly while testing this PR, not theoretical: the shim-refresh step silently stopped firing on --update once status grew a second line.

Fixed by capturing status's output into a variable before grepping it, in both install.sh call sites. A one-line status (as it was before this PR) never hits this, so the hazard was specifically introduced by this PR's own owner: line — worth calling out since it would otherwise have shipped as a silent, unrelated-looking breakage of the existing --update shim-refresh path.

Fixed in review: eval on shim-derived content was a code-execution path

Raised independently by two reviewers: shim_owner_script_dir extracted the recorded 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 at a local, normally-writable-by-that-user path that could have been hand-edited afterward. A file carrying the marker plus a line shaped like the real export ...= line, with a command substitution as its "value", turned a read-only status call into arbitrary code execution.

Fixed by dropping eval entirely. install now writes a dedicated, never-executed comment line (# agmsg-shim-owner: <dir>) alongside the real, executable export line the shim still needs at runtime for its own purposes, and shim_owner_script_dir reads only that comment via plain text extraction (sed) — no code path interprets file content as shell, regardless of what the file contains. Forced reproduction: reverted to the eval version, confirmed the new "tampered shim cannot execute code" test fails (RED) — the sentinel file it plants via a crafted command substitution actually gets created — then restored the fix and confirmed the sentinel is never created (GREEN) for both status and install.

Also strengthened per review: the refusal message named AGMSG_CODEX_SHIM_FORCE=1 but not what using it does. It now states explicitly that forcing repoints every Codex launch through the shim from the current owner to this install — and 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 run instead of being silently swallowed.

Fixed in review: an existing (pre-this-PR) shim looked "unowned" and got silently clobbered anyway

Raised in review: an 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 genuinely unknown, unrecorded ownership was treated as "unowned, safe to take" and silently overwritten by a 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 — i.e. most real machines, the first time they hit this.

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, differing only in the message shown (a named owner vs. "predates ownership tracking (#553)"). AGMSG_CODEX_SHIM_FORCE=1 still claims it either way. Forced reproduction: reverted the guard to its previous [ -n "$owner" ] form, confirmed the tampered-shim test's install assertion flips to succeeding-when-it-shouldn't (RED), restored the fix, confirmed it refuses (with the "predates ownership tracking" message) and only a forced install proceeds (GREEN).

Also folded in, non-blocking per review but cheap to close: the owner comment now stores the shell-%q-quoted path (matching the executable export line's own quoting) instead of 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.

Fixed in review: bare --update (no --cmd) forcing depended on a PR that isn't actually on this base

I originally reasoned that bare --update's force was safe because resolving to a single existing install is as unambiguous as naming it explicitly. Review measured that premise directly against the actual base branches rather than taking it on faith:

git show origin/integration/remote:install.sh | grep -c 'Several agmsg installs'   → 0
git show origin/main:install.sh              | grep -c 'Several agmsg installs'   → 0
gh pr view 659 --json state,baseRefName                                            → OPEN, base=main

#659 (the fail-closed fix for bare --update with multiple installs, #599) is not merged into either main or integration/remote. On this actual base, bare --update with more than one install present silently picks whichever sorts first from a glob (#599, still open) — so forcing the shim reclaim unconditionally meant an install the caller never named at all could steal the shim from another one, compounding #599 with a #553-shaped consequence. My reasoning would have been correct if #659 had landed; it hadn't, on the branch that matters.

Fixed by capturing whether --cmd was passed explicitly — CMD_WAS_EXPLICIT, set 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; a conflict now surfaces the same refusal message a fresh install would. Forced reproduction: reverted to unconditional forcing, confirmed the new "bare --update does NOT force-steal" test fails (RED) — a shim owned by one install gets silently reassigned to the other — restored the fix, confirmed it's left untouched (GREEN).

Fixed in review: the previous two fixes, combined, blocked the single-install upgrade they were never meant to touch

Raised in review: failing closed on an owner-unknown shim (previous section) and no longer forcing bare --update (the section before that) are each correct alone, but together they mean nearly every real machine — whose existing shim predates ownership tracking and therefore has no owner comment — could no longer have that shim refreshed by a routine install.sh --update with no --cmd, the normal way a single-install user upgrades. The shim would silently stay stale (potentially still pointing at a pre-types/ → scripts/drivers/types/-move path, the exact breakage the surrounding comment already exists to prevent) with no error and no indication anything needed attention.

Fixed by adding agmsg_only_one_install: true iff exactly one .agmsg-marked directory exists anywhere under ~/.agents/skills, derived directly from this machine's actual state (not assumed from whether #659 is merged, for the same "measure the base" reason as above). 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 it, 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 for an unrecorded owner 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 — restored the fix, confirmed it migrates to the new owner-tracked format (GREEN).

Test plan

Positive control, by hand, before writing the fix: installed a production copy (--cmd agmsg), installed its Codex shim, then installed a second, differently-named copy (--cmd agmsg-dfr) and confirmed byte-for-byte that the shim's recorded AGMSG_CODEX_SHIM_SCRIPT_DIR switched from the first install to the second.

New tests, forced RED (via git stash on just the production files) confirmed failing against the unfixed code, then GREEN with the fix restored:

• tests/test_install.bats:
  • install: a second, differently-named install does NOT clobber the first's Codex shim (#553) — pins the shim's exact recorded line unchanged, byte for byte, after the second install.
  • install: --update --cmd can reclaim a Codex shim owned by a different install (#553) — the documented recovery path still works.
  • install: bare --update (no --cmd) does NOT force-steal a Codex shim owned by a different install (#553) — pins the scoping fix: whichever install an ambiguous bare --update resolves to on this base, it must not walk away with a shim it was never explicitly told to claim.
  • install: bare --update migrates this machine's own pre-#553 (owner-unknown) Codex shim (#553) — pins the single-install allowance: the routine upgrade path a real, single-install user actually hits still works.
• tests/test_codex_shim.bats (unit-level, against codex-shim-install.sh directly, using two independent copies of the codex driver dir to stand in for two installs):
  • codex shim install: refuses to repoint a shim owned by a different install
  • codex shim install: AGMSG_CODEX_SHIM_FORCE=1 reclaims a shim owned by a different install
  • codex shim status: names which install currently owns the shim
  • codex shim install: a plain, non-agmsg codex binary is still refused regardless of ownership wording (#553 regression guard) — the pre-existing, unrelated "don't touch a real user binary" guard stays intact and isn't described as an ownership conflict.
  • codex shim status: a multi-line status output does not spuriously fail a piped grep -q check (#553 regression guard) — pins the SIGPIPE hazard above directly: documents that the naive piped form still fails today, and that the capture-first form install.sh actually uses does not.
  • codex shim status/install: a tampered shim cannot execute code via ownership parsing (#553 security regression guard) — plants a shim carrying the real marker plus a crafted line shaped like the old eval'd export, with a command substitution that writes a sentinel file if ever executed; confirms the sentinel never appears via status, a plain (refused) install, or a forced install. Doubles as the legacy-shim regression test: this crafted shim has no owner comment, so a plain install must refuse it (with the "predates ownership tracking" message), not silently claim it.

Full suite: bash -n install.sh / bash -n codex-shim-install.sh both clean. tests/test_codex_shim.bats + tests/test_install.bats: 73/73 pass.

Related

Read #599's fix (PR #659, still open against main, confirmed not merged into either main or integration/remote — see the bare---update fix above) before designing this, per instruction: same file (install.sh), same failure family ("a second install is invisible to / silently interferes with the first"). #659 fixes ambiguous candidate selection for a bare --update with multiple installs present (which install does an unqualified --update mean); this PR fixes ownership verification for the shim specifically, regardless of how the target install was selected. The two touch different lines/logic within the same file and don't conflict — #659 never reaches the shim-refresh block's own condition, it only changes which SKILL_DIR gets resolved before that block runs. This PR's own safety no longer assumes #659 landed (see above) — if #659 does land first, CMD_WAS_EXPLICIT still works unchanged, since it only depends on whether the caller typed --cmd, not on how ambiguity is otherwise handled.

Known, accepted overlap with #659 (flagged in review, non-blocking): agmsg_only_one_install (codex-shim-install.sh) and #659's own bare---update candidate enumeration (not written yet, since #659 is unmerged) both answer the same question — how many agmsg installs exist on this machine — by independently scanning ~/.agents/skills for .agmsg markers. This is duplication, not conflict: the two never call each other or share state today, and nothing in either implementation depends on the other's presence or absence. To leave a path toward unifying them later without needing changes on #659's side, the scan was split out into agmsg_install_candidates (lists the candidate directories) with agmsg_only_one_install now just counting its output — so once #659 lands on a shared base with this file, its enumeration could call agmsg_install_candidates instead of re-implementing the same scan. Full unification wasn't done here: #659's counting logic doesn't exist on this base yet, so there is nothing concrete to point it at.

Fixed in review: self wasn't counted as a candidate until its own marker existed

Raised in review, independently verified against the code (install.sh:436 checks/refreshes the shim, install.sh:452 touches this install's own .agmsg marker — the marker comes after): during a fresh, second, differently-named install, agmsg_only_one_install ran before that install's own marker was written. It only saw the FIRST install's marker on disk, concluded "only one install exists anywhere", and let the second install silently claim a legacy (owner-unknown) shim without --force — reintroducing #553's own bug specifically for the single-legacy-shim case the previous round's fix was meant to preserve.

Fixed by counting other installs relative to self (self's own skill root, recovered from SCRIPT_DIR by stripping the fixed .../scripts/drivers/types/codex suffix install.sh always lays this script out under) instead of a plain total. Self now 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. this script run 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 fresh-install-level regression test, confirmed it fails (RED) against the self-unaware version — the second install's shim step reads the first install's lone marker and silently repoints — restored the fix, confirmed GREEN. Also added direct unit coverage for the boundary (self invisible with zero/one other install, self already registered with a space in its name, an unmarked sibling correctly excluded), each constructing a realistic $0 via bash -c's argv0 trick so SCRIPT_DIR resolves the way it does in a real invocation.

Fixed in review: this round's own test additions broke the enforced-assertions gate

.github/scripts/check-enforced-assertions.sh counts non-last [[ ]]/(( ))/! cmd assertions in bats tests, because a false one in that position can't fail the suite on macOS bash 3.2 (#670) — silently indistinguishable from a check that was never written. This round's new/rewritten tests pushed the count 18 over the 616 baseline. Converted the flagged lines to grep -qF/refute (both plain commands, always enforced regardless of position) or left as [ ], which the checker never flags. No behavior change to any assertion; count is back to exactly 616.

Known, accepted overlap with #659 (flagged in review, non-blocking)

agmsg_only_one_install (codex-shim-install.sh) and #659's own bare---update candidate enumeration (not written yet, since #659 is unmerged) both answer the same question — how many agmsg installs exist on this machine — by independently scanning ~/.agents/skills for .agmsg markers. This is duplication, not conflict: the two never call each other or share state today, and nothing in either implementation depends on the other's presence or absence. To leave a path toward unifying them later without needing changes on #659's side, the scan was split into agmsg_install_candidates (lists candidate directories) with agmsg_only_one_install counting its output — so once #659 lands on a shared base with this file, its enumeration could call agmsg_install_candidates instead of re-implementing the same scan. Full unification wasn't done here: #659's counting logic doesn't exist on this base yet, so there is nothing concrete to point it at.

base: integration/remote. Not merging — awaiting review.