fix(install): refuse to guess between multiple installs on --update with no --cmd - #659
Open
fujibee wants to merge 3 commits into
Open
fix(install): refuse to guess between multiple installs on --update with no --cmd#659fujibee wants to merge 3 commits into
fujibee wants to merge 3 commits into
Conversation
--update with no --cmd The skill-directory selection loop broke on the first .agmsg- marked directory a glob yielded. A glob expands in collation order, not installation order, and nothing records which install came first, so the "historical first installed" comment described an invariant the code could not observe. On a machine with more than one install, --update silently touched whichever one sorted first -- including the shared ~/.agents/bin/codex shim it refreshes -- while the intended install, and the caller, saw nothing wrong. Enumerate all .agmsg-marked directories instead of stopping at the first. Zero or one candidate behaves exactly as before -- the common single-install case is unaffected. Two or more now fails closed: list the candidates and ask for --cmd, rather than guessing. Backup-shaped directory names (*.bak*) are excluded from the candidate set, since they carry the same .agmsg marker as a real install but are never the one a bare --update means. Fixes #599.
missed-owner test co2 review on #659: the "*.bak*" exclusion pattern also matched a real install whose --cmd name merely contains "bak" (e.g. "agmsg.bakery") -- --cmd has no reserved-name validation, so that install silently dropped out of the candidate set, reproducing the same silent-wrong-pick failure #599 exists to close. Narrow the pattern to the literal ".bak-" shape actually reported (agmsg.bak-20260731), which does not collide with a plausible command name. Also tighten the two-installs regression test: it was comparing VERSION strings, which are the same source-derived value for both installs and would not distinguish "one got silently updated" from "neither did". Compare distinct per-install sentinels instead.
closed
Two rounds of narrowing ("*.bak*", then "*.bak-*") hit the same
collision: --cmd has no reserved-name validation, so any pattern
that catches a real backup name (e.g. "agmsg.bak-20260731") can
equally match a legitimately chosen install name (e.g.
"agmsg.bak-tool" or "agmsg.bakery"), silently dropping a real
install from the candidate set -- the exact failure #599 exists to
close, just moved into the exclusion instead of the glob order.
No code in this repo generates a ".bak-"-named directory; the
reported name is a human backup convention, not something the
installer produces, so no name-based pattern can be made safe here.
Stop special-casing names. A directory that still carries the
.agmsg marker is just another candidate; more than one candidate is
exactly the ambiguity this fix already refuses to guess through.
The regression test now shows a leftover backup-shaped directory
landing in the same "Several agmsg installs found" refusal as any
other second candidate, rather than being silently excluded or
silently chosen.
fujibee
added a commit
that referenced
this pull request
Aug 10, 2026
…cing (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).
fujibee
added a commit
that referenced
this pull request
Aug 10, 2026
… (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).
fujibee
added a commit
that referenced
this pull request
Aug 10, 2026
…all (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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #599.
install.sh --updatewith no--cmdused to stop at the first.agmsg-marked directory a glob yielded. A glob expands in collation order, not installation order, and nothing records which install came first, so the comment describing this as preserving "the historical first installed agmsg skill" behavior was describing an invariant the code has no way to observe. On a machine with more than one install,--updatesilently touched whichever one sorted first -- including the shared~/.agents/bin/codexshim it refreshes -- while the install the caller actually meant, and the caller, saw nothing wrong.What changed
install.sh's update-mode skill-directory selection now enumerates every.agmsg-marked directory instead of stopping at the first:--cmd <name>instead of guessing, with a nonzero exit.*.bak*) are excluded from the candidate set. They carry the same.agmsgmarker as a real install (it gets copied along with everything else) but are never the one an unqualified--updatemeans.--cmd <name>continues to target exactly the named install regardless of how many others exist, unchanged.Tests
Two new cases in
tests/test_install.bats, alongside the existing--update --cmdcoverage:install: --update with no --cmd refuses to guess between two real installs (#599)-- two real installs present, bare--updateexits nonzero, lists both names, and neither install'sVERSIONchanges.install: --update with no --cmd still finds the one real install past a bak-named decoy (#599)-- one real install plus a.bak--named directory carrying the same marker; bare--updatestill finds and updates the real one, and the decoy is untouched.Full
tests/test_install.batsrun: 49/49 ok, exit 0 (includes the two new tests and the pre-existing--update --cmd+ backup-skill case, unaffected since it always specifies--cmd).bash -n install.shandgit diff --checkboth pass. Manually smoke-tested end to end against an isolated tempHOMEwith two real installs (fails closed as expected) and a single install (updates normally).