From 3970f7f4e4893ae7528723809329dcdfeb2015ff Mon Sep 17 00:00:00 2001 From: KageBinary Date: Thu, 1 Oct 2026 18:39:50 -0700 Subject: [PATCH 1/2] fix(installer): upgrade over any earlier install without --force Re-running the installer after any change raised FileExistsError, after part of the install had been written, and neither hosted one-liner passes --force. Even with --force, `ix mcp install` ran without it, unbounded and unchecked, so a 2.4.1 `ix-memory` entry pointing at the deleted mcp/server.py was never replaced. The installer now plans every change, checks it, stages the new files beside their destinations and swaps them in together, restoring the previous files if any swap fails: - files the plugin owns are overwritten, and ones it no longer ships are removed; ownership is recorded in ix-plugin-version.json (`files`), with the historical file list for installs that predate it. The plugin tree is replaced whole, so dropped skills go too. - hooks.json is merged: plugin handlers from any version (`-lc`, `-c`, `Bash` matchers, the 2.4.x Windows launcher form) are replaced where the first one was, and every other hook is kept in place. - the marketplace entry is replaced when it points at the plugin's own path; another ix-memory entry still needs --force. - `ix mcp install --host codex --format json` runs with a timeout; its exit status and per-host outcome are reported as warnings, never a crash. A registration of the old mcp/server.py is replaced with --force (or, for a CLI without it, by rewriting that one table in a checked copy of config.toml), and the old server files are removed once nothing launches them. - the pre-2.4.2 /tmp/ix-codex-hooks cache is removed when it is the user's own and holds only cache files. - the hook-trust notice is printed only when hooks.json changed. Windows: Codex 0.155 runs hook commands through the session shell, which on Windows is `powershell.exe -NoProfile -Command`. The 2.4.x command `"python" "launcher" name` is a parse error there. Each hook now gets a `commandWindows` of `& '' '' `, and `command` keeps the POSIX form. Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 24 +- hooks.md | 1 + install.ps1 | 3 +- install.sh | 1 + scripts/install_codex_integration.py | 1265 +++++++++++++++++++++----- test-local.sh | 8 + tests/test_installer_upgrade.py | 569 ++++++++++++ tests/test_ix_argv_resolution.py | 5 +- tests/test_windows_hook_launch.py | 101 +- 9 files changed, 1674 insertions(+), 303 deletions(-) create mode 100644 tests/test_installer_upgrade.py diff --git a/README.md b/README.md index 0000ab0..a0998dd 100644 --- a/README.md +++ b/README.md @@ -171,6 +171,26 @@ Hooks: MCP: - registered with Codex as `ix-memory`, served by the Ix CLI (`ix mcp`) — no file is installed +### Upgrading + +Re-run the same installer; no flags are needed. Over any earlier version it: + +- overwrites the files the plugin owns and removes the ones it no longer ships (recorded in + `.codex/ix-plugin-version.json`), and never touches anything else in `.codex/`; +- merges into an existing `hooks.json`: the plugin's entries from any earlier version are + replaced in place, and every other hook in the file is kept; +- checks everything before writing, then swaps the new files in together — a failed install + leaves the previous one exactly as it was; +- runs `ix mcp install --host codex` with a time limit and reports how it went. A registration + of the `mcp/server.py` that 2.4.1 and earlier shipped is replaced (and the old file removed); + any other server already named `ix-memory` is left alone unless you pass `--force`; +- removes the `/tmp/ix-codex-hooks` cache older hooks used, if it is yours and holds only cache files. + +If `hooks.json` changed, Codex asks you to trust the hooks again on its next start. + +`--force` is only for replacing what is not the plugin's: a marketplace entry named `ix-memory` +that points elsewhere, a `hooks.json` that is not valid JSON, or a different `ix-memory` MCP server. + ### Home-local install ```bash @@ -270,13 +290,13 @@ ix-memory plugin v2.3.0 active | installed 2026-04-29 | commit a1b2c3d **To confirm a reinstall took effect:** -1. Reinstall: `./install.sh --repo /path/to/project --hooks --force` +1. Reinstall: `./install.sh --repo /path/to/project --hooks` 2. Check the new timestamp: `cat /path/to/project/.codex/ix-plugin-version.json` 3. Start a new Codex session — the version line in the session context will reflect the new install date and commit. If the version line is missing, the hooks were installed manually without using the installer. -Run `./install.sh --repo /path/to/project --hooks --force` to write the file. +Run `./install.sh --repo /path/to/project --hooks` to write the file. ## Repo Guidance diff --git a/hooks.md b/hooks.md index 23e9958..413da04 100644 --- a/hooks.md +++ b/hooks.md @@ -19,6 +19,7 @@ Verified against the `codex` 0.155.1 binary and `openai/codex@rust-v0.155.1` (`c - **Model context.** `hookSpecificOutput.additionalContext` (with `hookEventName` set to the event) is the only output Codex adds to the model's input on `SessionStart`, `UserPromptSubmit`, `PreToolUse` and `PostToolUse`. A top-level `systemMessage` becomes a UI warning only. `Stop` has no model channel; its output is `{"continue": true}`. The output structs deny unknown fields, so a misspelt key fails the hook run. `tests/test_codex_hook_protocol.py` validates every hook against the generated schemas, vendored in `tests/fixtures/codex-0.155.1/`. - **Tool names.** Shell tools reach hooks as `Bash`. File edits use `apply_patch`, which Codex also matches as `Edit` and `Write`, with the raw patch as `tool_input.command`. Both pre and post tool hooks match `Bash|apply_patch`. The touched files are read from the patch's `*** Add File:` / `Update File:` / `Delete File:` / `Move to:` headers. - **Shell.** Codex runs each `command` through the session shell without a login profile (` -c`). The inner `/bin/sh -c` stays non-login too. A login shell would source the user's profile on every hook, and anything that profile prints corrupts the hook's JSON. +- **Windows.** The session shell there is PowerShell (`shell-command/src/shell_detect.rs`, `default_user_shell`), so a hook runs as `powershell.exe -NoProfile -Command ` (`core/src/session/mod.rs` builds the hook shell with `derive_exec_args`). The installer gives each Ix hook a `commandWindows` of `& '' '<...\.codex\hooks\_launch.py>' `: without `&`, a command that starts with a quoted path is a string expression to PowerShell, which is why the 2.4.x form `"python" "launcher" name` never ran. `command` keeps the POSIX form, so a repo-local `hooks.json` still works on macOS and Linux. Only when a session has no shell does Codex fall back to `%COMSPEC% /C`, where `&` is not valid. - **Feature flag.** `hooks` has been stable and on by default since Codex 0.124 (openai/codex#19012). `codex_hooks` is a deprecated alias (`codex-rs/features/src/legacy.rs`), so the installer no longer writes it. - **Trust.** User (`~/.codex/hooks.json`), project (`.codex/hooks.json`) and plugin-bundled hooks are all non-managed. Codex skips them until their exact definition is trusted, records trust as a `trusted_hash` of the normalised hook (event, matcher, command, timeout, statusMessage), and asks again whenever that hash changes (`codex-rs/hooks/src/engine/discovery.rs`). The TUI prompts "Hooks need review" at startup. `codex exec` cannot prompt, so it skips untrusted hooks unless it is run with `--dangerously-bypass-hook-trust`. Changing `hooks.json`, as this release does, requires the user to review the hooks again. diff --git a/install.ps1 b/install.ps1 index 103d497..f75ff50 100644 --- a/install.ps1 +++ b/install.ps1 @@ -29,11 +29,12 @@ Examples: Flags: --plugin Copy/register the ix-memory Codex plugin in a local marketplace --hooks Install the .codex hook bundle (session, prompt, pre/post tool, stop) - --mcp Install the ix-memory MCP server and print the codex mcp add command + --mcp Register the Ix CLI's MCP server (`ix mcp`) with Codex Notes: - If none of --plugin, --hooks, or --mcp is passed, the installer defaults to --plugin --hooks --mcp. + - Re-running upgrades in place; --force is not needed for that. - --plugin does not activate the plugin in Codex. Restart Codex, then install or enable 'ix-memory' from the marketplace before its skills appear. - This wrapper forwards all arguments to scripts/install_codex_integration.py. diff --git a/install.sh b/install.sh index 36c019c..7dc2a15 100755 --- a/install.sh +++ b/install.sh @@ -29,6 +29,7 @@ Flags: Notes: - If none of --plugin, --hooks, or --mcp is passed, the installer defaults to --plugin --hooks --mcp. + - Re-running upgrades in place; --force is not needed for that. - --plugin does not activate the plugin in Codex. Restart Codex, then install or enable 'ix-memory' from the marketplace before its skills appear. - Local Codex plugins do not reliably expose skill autocomplete, so type `$ix-tutorial` diff --git a/scripts/install_codex_integration.py b/scripts/install_codex_integration.py index 94bac90..8864b4c 100644 --- a/scripts/install_codex_integration.py +++ b/scripts/install_codex_integration.py @@ -1,16 +1,38 @@ #!/usr/bin/env python3 # Copyright 2026 Ix Infrastructure Inc. +"""Install the ix-memory plugin, its Codex hooks and its MCP registration. + +Re-running this is how users upgrade, and the hosted one-liners never pass +`--force`, so a re-run over any earlier version has to just work: + +* files the plugin owns are overwritten, and ones it no longer ships are + removed. Ownership is recorded in `.codex/ix-plugin-version.json` (`files`); + installs that predate the record are recognised by the file names every + earlier version shipped; +* `hooks.json` is merged, not copied: the plugin's own entries -- from any + earlier version, whatever their command string or matcher -- are replaced in + place, and every other hook in the file is kept; +* nothing is written until everything has been read and checked, and the writes + themselves are staged beside their destinations and swapped in together, with + the previous files restored if any swap fails. An install either lands whole + or leaves the target as it found it. +""" + from __future__ import annotations import argparse +import copy import datetime import json import os import re import shutil +import stat import subprocess import sys +import tempfile +import uuid from pathlib import Path try: # 3.11+ @@ -19,8 +41,9 @@ try: import tomli as tomllib # type: ignore[no-redef] except ModuleNotFoundError: - # Only used to read config.toml for a hooks-disabled warning, which is - # skipped without a parser. + # Only read: for a hooks-disabled warning, to recognise a stale MCP + # registration, and to prove a rewritten config still parses. Each of + # those is skipped, never guessed at, without a parser. tomllib = None # type: ignore[assignment] @@ -35,10 +58,36 @@ "category": "Productivity", } +VERSION_FILE = Path(".codex") / "ix-plugin-version.json" + +# Every file an install made before the version file recorded `files`. 2.4.1 and +# earlier also copied an MCP server into `.codex/mcp/`; those two are handled by +# remove_legacy_mcp_files, after the registration that may still point at them. +LEGACY_HOOK_FILES = tuple( + f".codex/hooks/{name}.py" + for name in ( + "_launch", + "common", + "post_tool_use", + "pre_tool_use", + "session_start", + "stop", + "user_prompt_submit", + ) +) +LEGACY_MCP_FILES = (".codex/mcp/server.py", ".codex/mcp/ix_llm.py") + + +class InstallError(Exception): + """A problem found before, or while, writing. Reported without a traceback.""" -def parse_args() -> argparse.Namespace: + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: parser = argparse.ArgumentParser( - description="Install the ix-codex-plugin package, hooks, and MCP into a repo or home config." + description=( + "Install or upgrade the ix-codex-plugin package, hooks, and MCP registration " + "in a repo or home config. Re-running it upgrades in place." + ) ) target_group = parser.add_mutually_exclusive_group(required=True) target_group.add_argument("--repo", help="Target repository root") @@ -71,7 +120,11 @@ def parse_args() -> argparse.Namespace: parser.add_argument( "--force", action="store_true", - help="Overwrite conflicting files and replace an existing marketplace entry", + help=( + "Also replace what is not the plugin's: a marketplace entry named ix-memory " + "that points elsewhere, an unreadable hooks.json, and an ix-memory MCP " + "server that is not `ix mcp`. Not needed to upgrade." + ), ) parser.add_argument( "--marketplace-name", @@ -83,7 +136,7 @@ def parse_args() -> argparse.Namespace: default=DEFAULT_MARKETPLACE_DISPLAY_NAME, help="Marketplace display name to create if the target has no marketplace yet", ) - args = parser.parse_args() + args = parser.parse_args(argv) if not args.plugin and not args.hooks and not args.mcp: args.plugin = True args.hooks = True @@ -113,38 +166,6 @@ def same_link(destination: Path, source: Path) -> bool: return destination.is_symlink() and destination.resolve() == source.resolve() -def same_file_contents(source: Path, destination: Path) -> bool: - return destination.exists() and source.read_bytes() == destination.read_bytes() - - -def remove_path(path: Path) -> None: - if path.is_symlink() or path.is_file(): - path.unlink() - elif path.is_dir(): - shutil.rmtree(path) - - -def install_file(source: Path, destination: Path, mode: str, force: bool) -> None: - destination.parent.mkdir(parents=True, exist_ok=True) - - if mode == "symlink": - if destination.exists() or destination.is_symlink(): - if same_link(destination, source): - return - if not force: - raise FileExistsError(f"{destination} already exists. Re-run with --force.") - remove_path(destination) - destination.symlink_to(source) - return - - if destination.exists(): - if same_file_contents(source, destination): - return - if not force: - raise FileExistsError(f"{destination} already exists. Re-run with --force.") - shutil.copy2(source, destination) - - def _same_path(left: Path, right: Path) -> bool: try: return left.resolve() == right.resolve() @@ -152,63 +173,300 @@ def _same_path(left: Path, right: Path) -> bool: return False -def install_rendered(destination: Path, content: str, force: bool) -> None: - """install_file for content generated here rather than copied from the tree. +def _same_location(left: Path, right: Path) -> bool: + """True when both name one directory entry, as installing a checkout into itself does. - No symlink mode: the caller reaches this precisely because the file it needs - differs from the source, and a link would point at the wrong bytes. An - existing symlink is replaced -- rewriting through it would edit the checkout. + The parents are resolved, the entry itself is not: a symlink an earlier + `--mode symlink` install left at the destination points at the source, but + is not the source, and has to be replaceable. """ - destination.parent.mkdir(parents=True, exist_ok=True) - desired = content.encode("utf-8") + return left.name == right.name and _same_path(left.parent, right.parent) - if destination.is_symlink(): - destination.unlink() - elif destination.exists(): - if destination.read_bytes() == desired: - return - if not force: - raise FileExistsError(f"{destination} already exists. Re-run with --force.") - destination.write_bytes(desired) +def _lexists(path: Path) -> bool: + return path.is_symlink() or path.exists() -def install_tree(source_dir: Path, destination_dir: Path, mode: str, force: bool) -> None: - for source in sorted(source_dir.rglob("*")): - relative = source.relative_to(source_dir) - destination = destination_dir / relative - if source.is_dir(): - destination.mkdir(parents=True, exist_ok=True) - continue - install_file(source, destination, mode, force) +def remove_path(path: Path) -> None: + if path.is_symlink() or path.is_file(): + path.unlink() + elif path.is_dir(): + shutil.rmtree(path) + + +def _json_bytes(payload: dict) -> bytes: + # ensure_ascii=False: the shipped manifests carry an em dash, and escaping it + # would make every rewrite differ from the file it came from. + return (json.dumps(payload, indent=2, ensure_ascii=False) + "\n").encode("utf-8") -def load_json(path: Path) -> dict: - with path.open() as handle: - payload = json.load(handle) +def _read_json_object(path: Path) -> dict: + """`path` as a JSON object, or InstallError saying why it is not one. + + utf-8-sig, not utf-8: PowerShell 5.1's Set-Content writes a BOM by default, + and json.loads rejects one. + """ + try: + payload = json.loads(path.read_text(encoding="utf-8-sig")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + raise InstallError(f"{path} could not be read as JSON ({exc}).") from exc if not isinstance(payload, dict): - raise ValueError(f"{path} must contain a JSON object.") + raise InstallError(f"{path} must contain a JSON object.") return payload -def write_json(path: Path, payload: dict) -> None: - path.parent.mkdir(parents=True, exist_ok=True) - with path.open("w") as handle: - json.dump(payload, handle, indent=2) - handle.write("\n") +# -------------------------------------------------------------------------- +# Staged, all-or-nothing writes +# -------------------------------------------------------------------------- + +# The one call every swap goes through, so a test can fail one partway. +_replace = os.replace + + +class _Op: + """One planned change: write bytes, place a symlink, replace a tree, or remove.""" + + def __init__( + self, + dest: Path, + kind: str, + data: bytes = b"", + source: Path | None = None, + tree: dict[str, object] | None = None, + ) -> None: + self.dest = dest + self.kind = kind # "bytes" | "link" | "tree" | "remove" + self.data = data + self.source = source + self.tree = tree or {} + self.staged: Path | None = None + self.backup: Path | None = None + self.placed = False + + +class Transaction: + """Collect writes, then apply them all or none. + + Planning reads only. `commit` first writes every new file to a sibling of its + destination -- the same directory, so the same filesystem, so the rename that + follows is atomic on POSIX and Windows alike -- and only then swaps them in, + moving each current file aside first. If any step fails, the swapped-in files + are removed, the originals are moved back, and the error is raised with the + target as it was. + + Moving the old file aside rather than writing over it is also what keeps a + symlink from an earlier `--mode symlink` install from being written through + into the plugin checkout it points at. + """ + + def __init__(self) -> None: + self.token = f"{os.getpid()}-{uuid.uuid4().hex[:8]}" + self.ops: list[_Op] = [] + self._created_dirs: list[Path] = [] + + # -- planning --------------------------------------------------------- + + def _add(self, op: _Op) -> None: + self.ops = [existing for existing in self.ops if existing.dest != op.dest] + self.ops.append(op) + + def write(self, dest: Path, data: bytes) -> bool: + if not dest.is_symlink() and dest.is_file(): + try: + if dest.read_bytes() == data: + return False + except OSError: + pass + self._add(_Op(dest, "bytes", data=data)) + return True + + def link(self, dest: Path, source: Path) -> bool: + if same_link(dest, source): + return False + self._add(_Op(dest, "link", source=source)) + return True + + def install(self, dest: Path, source: Path, mode: str) -> bool: + if mode == "symlink": + return self.link(dest, source) + return self.write(dest, source.read_bytes()) + + def tree(self, dest: Path, source_dir: Path, mode: str) -> bool: + """Replace the directory `dest` with a copy (or links) of `source_dir`. + + Whole-directory replacement is what removes a file an earlier version + shipped and this one does not, such as a dropped skill. + """ + desired: dict[str, object] = {} + for source in sorted(source_dir.rglob("*")): + if source.is_dir(): + continue + relative = source.relative_to(source_dir).as_posix() + desired[relative] = source if mode == "symlink" else source.read_bytes() + if self._tree_matches(dest, desired): + return False + self._add(_Op(dest, "tree", tree=desired)) + return True + + @staticmethod + def _tree_matches(dest: Path, desired: dict[str, object]) -> bool: + if dest.is_symlink() or not dest.is_dir(): + return False + present = { + path.relative_to(dest).as_posix(): path + for path in dest.rglob("*") + if path.is_symlink() or not path.is_dir() + } + if set(present) != set(desired): + return False + for relative, want in desired.items(): + have = present[relative] + if isinstance(want, Path): + if not same_link(have, want): + return False + elif have.is_symlink() or have.read_bytes() != want: + return False + return True + + def remove(self, dest: Path) -> bool: + if not _lexists(dest): + return False + self._add(_Op(dest, "remove")) + return True + + @property + def changed(self) -> list[Path]: + return [op.dest for op in self.ops] + + def changes(self, path: Path) -> bool: + return any(op.dest == path for op in self.ops) + + # -- applying --------------------------------------------------------- + + def _sibling(self, dest: Path, tag: str) -> Path: + return dest.parent / f".{dest.name}.ix-{tag}-{self.token}" + + def _mkdirs(self, directory: Path) -> None: + missing: list[Path] = [] + probe = directory + while not probe.exists(): + missing.append(probe) + if probe.parent == probe: + break + probe = probe.parent + for path in reversed(missing): + path.mkdir() + self._created_dirs.append(path) + + def _stage(self, op: _Op) -> None: + if op.kind == "remove": + return + self._mkdirs(op.dest.parent) + staged = self._sibling(op.dest, "new") + op.staged = staged + if op.kind == "bytes": + with open(staged, "xb") as handle: + handle.write(op.data) + return + if op.kind == "link": + assert op.source is not None + staged.symlink_to(op.source) + return + staged.mkdir() + for relative, want in op.tree.items(): + path = staged / relative + path.parent.mkdir(parents=True, exist_ok=True) + if isinstance(want, Path): + path.symlink_to(want) + else: + path.write_bytes(want) # type: ignore[arg-type] + + def commit(self) -> list[Path]: + try: + for op in self.ops: + self._stage(op) + for op in self.ops: + if _lexists(op.dest): + backup = self._sibling(op.dest, "old") + _replace(op.dest, backup) + op.backup = backup + if op.staged is not None: + _replace(op.staged, op.dest) + op.staged = None + op.placed = True + except BaseException as exc: + problems = self._rollback() + detail = f"{type(exc).__name__}: {exc}" + if problems: + raise InstallError( + f"Install failed ({detail}) and could not be fully undone:\n " + + "\n ".join(problems) + ) from exc + raise InstallError( + f"Install failed ({detail}). Nothing was changed; the previous files are in place." + ) from exc + self._finish() + return self.changed + + def _rollback(self) -> list[str]: + problems: list[str] = [] + for op in reversed(self.ops): + try: + if op.placed: + remove_path(op.dest) + op.placed = False + if op.backup is not None: + _replace(op.backup, op.dest) + op.backup = None + except OSError as exc: + problems.append(f"{op.dest}: {exc}") + if op.staged is not None: + try: + if _lexists(op.staged): + remove_path(op.staged) + except OSError: + pass + for directory in reversed(self._created_dirs): + try: + directory.rmdir() + except OSError: + pass + return problems + + def _finish(self) -> None: + for op in self.ops: + if op.backup is None: + continue + try: + remove_path(op.backup) + except OSError as exc: # pragma: no cover - e.g. a file held open on Windows + print(f" [!!] could not remove {op.backup}: {exc}", file=sys.stderr) + + +# -------------------------------------------------------------------------- +# Plugin + marketplace +# -------------------------------------------------------------------------- def _read_plugin_json() -> dict: plugin_json = source_plugin_dir() / ".codex-plugin" / "plugin.json" try: - with plugin_json.open() as fh: + with plugin_json.open(encoding="utf-8") as fh: meta = json.load(fh) return meta if isinstance(meta, dict) else {} except (OSError, json.JSONDecodeError): return {} -def update_marketplace( +def _normalise_local_path(value: object) -> str: + text = str(value or "").replace("\\", "/") + while text.startswith("./"): + text = text[2:] + return text.rstrip("/") + + +def plan_marketplace( + tx: Transaction, marketplace_path: Path, plugin_path: str, marketplace_name: str, @@ -217,24 +475,26 @@ def update_marketplace( version: str = "", description: str = "", ) -> None: - if marketplace_path.exists(): - payload = load_json(marketplace_path) + if _lexists(marketplace_path): + payload = _read_json_object(marketplace_path) + original = copy.deepcopy(payload) else: payload = { "name": marketplace_name, "interface": {"displayName": marketplace_display_name}, "plugins": [], } + original = None payload.setdefault("name", marketplace_name) interface = payload.setdefault("interface", {}) if not isinstance(interface, dict): - raise ValueError(f"{marketplace_path} field 'interface' must be an object.") + raise InstallError(f"{marketplace_path} field 'interface' must be an object.") interface.setdefault("displayName", marketplace_display_name) plugins = payload.setdefault("plugins", []) if not isinstance(plugins, list): - raise ValueError(f"{marketplace_path} field 'plugins' must be an array.") + raise InstallError(f"{marketplace_path} field 'plugins' must be an array.") new_entry: dict = { "name": PLUGIN_NAME, @@ -250,43 +510,89 @@ def update_marketplace( new_entry["description"] = description for index, entry in enumerate(plugins): - if isinstance(entry, dict) and entry.get("name") == PLUGIN_NAME: - if entry == new_entry: - write_json(marketplace_path, payload) - return - if not force: - raise FileExistsError( - f"{marketplace_path} already has a plugin entry named '{PLUGIN_NAME}'. " - "Re-run with --force to replace it." - ) - plugins[index] = new_entry - write_json(marketplace_path, payload) - return + if not (isinstance(entry, dict) and entry.get("name") == PLUGIN_NAME): + continue + source = entry.get("source") + existing_path = source.get("path") if isinstance(source, dict) else None + ours = _normalise_local_path(existing_path) == _normalise_local_path(plugin_path) + if not ours and not force: + raise InstallError( + f"{marketplace_path} already has a plugin named '{PLUGIN_NAME}' at " + f"{existing_path!r}, not {plugin_path!r}. Re-run with --force to replace it." + ) + plugins[index] = entry if entry == new_entry else new_entry + break + else: + plugins.append(new_entry) - plugins.append(new_entry) - write_json(marketplace_path, payload) + if payload != original: + tx.write(marketplace_path, _json_bytes(payload)) +def plan_plugin( + tx: Transaction, + target_root: Path, + home_install: bool, + mode: str, + force: bool, + marketplace_name: str, + marketplace_display_name: str, +) -> list[Path]: + if home_install: + plugin_destination = target_root / ".codex" / "plugins" / PLUGIN_NAME + plugin_path = f"./.codex/plugins/{PLUGIN_NAME}" + else: + plugin_destination = target_root / "plugins" / PLUGIN_NAME + plugin_path = f"./plugins/{PLUGIN_NAME}" + + if not _same_location(plugin_destination, source_plugin_dir()): + # Installing the checkout into itself: the tree is already the source. + tx.tree(plugin_destination, source_plugin_dir(), mode) + + plugin_meta = _read_plugin_json() + marketplace_path = target_root / ".agents" / "plugins" / "marketplace.json" + plan_marketplace( + tx, + marketplace_path, + plugin_path, + marketplace_name, + marketplace_display_name, + force, + version=str(plugin_meta.get("version", "")), + description=str(plugin_meta.get("description", "")), + ) + return [plugin_destination, marketplace_path] + + +# -------------------------------------------------------------------------- +# config.toml (read-only, except to replace a stale MCP registration) +# -------------------------------------------------------------------------- + # Codex 0.124 made lifecycle hooks a stable, default-on feature named `hooks` # (openai/codex#19012). `codex_hooks`, which this installer used to write, is now # a legacy alias (codex-rs/features/src/legacy.rs) that Codex reports as -# deprecated on every start. So nothing is written to config.toml any more; it is -# only read, to say so when the user's config turns hooks off. +# deprecated on every start. So nothing is written to config.toml for hooks; it +# is only read, to say so when the user's config turns hooks off. _HOOK_FEATURE_KEYS = ("hooks", "codex_hooks") +def _load_toml(config_path: Path) -> dict | None: + if tomllib is None: + return None + try: + return tomllib.loads(config_path.read_text(encoding="utf-8-sig")) + except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): + return None + + def hooks_disabled_by(config_path: Path) -> str | None: """The `[features]` key in `config_path` that turns hooks off, if any. Read-only, and silent on anything it cannot read: a config this cannot parse is Codex's to report, not a reason to fail an install. """ - if tomllib is None: - return None - try: - features = tomllib.loads(config_path.read_text(encoding="utf-8-sig")).get("features") - except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): - return None + config = _load_toml(config_path) + features = config.get("features") if config else None if not isinstance(features, dict): return None for key in _HOOK_FEATURE_KEYS: @@ -295,6 +601,106 @@ def hooks_disabled_by(config_path: Path) -> str | None: return None +def legacy_hooks_flag(config_path: Path) -> bool: + """True when config.toml still carries the `codex_hooks = true` 2.4.2 and earlier wrote.""" + config = _load_toml(config_path) + features = config.get("features") if config else None + return isinstance(features, dict) and features.get("codex_hooks") is True + + +def codex_config_path() -> Path: + """The config `codex mcp add` -- and so `ix mcp install` -- writes to.""" + codex_home = os.environ.get("CODEX_HOME", "") + if codex_home and os.path.isabs(codex_home): + return Path(codex_home) / "config.toml" + return Path.home() / ".codex" / "config.toml" + + +_LEGACY_SERVER_RE = re.compile(r"(^|[/\\])mcp[/\\]server\.py$") + + +def stale_mcp_registration(config_path: Path) -> str | None: + """The old server an `ix-memory` registration points at, if it is 2.4.1 or earlier's. + + 2.4.1 and earlier copied a FastMCP server to `.codex/mcp/server.py` and told + the user to run `codex mcp add ix-memory -- python3 `. `ix mcp + install` reads such an entry as a different server and, without `--force`, + leaves it in place -- so Codex kept launching a file the plugin had deleted. + """ + config = _load_toml(config_path) + servers = config.get("mcp_servers") if config else None + entry = servers.get(PLUGIN_NAME) if isinstance(servers, dict) else None + if not isinstance(entry, dict): + return None + args = entry.get("args") + words = [entry.get("command")] + (list(args) if isinstance(args, list) else []) + for word in words: + if isinstance(word, str) and _LEGACY_SERVER_RE.search(word.strip()): + return word.strip() + return None + + +_MCP_TABLE_RE = re.compile( + r"""^[ \t]*\[[ \t]*mcp_servers[ \t]*\.[ \t]*(?:ix-memory|"ix-memory"|'ix-memory')[ \t]*\][ \t]*(?:\#.*)?$""" +) +_ANY_TABLE_RE = re.compile(r"^[ \t]*\[") + + +def rewrite_mcp_registration(config_path: Path, command: str, args: list[str]) -> bool: + """Point `[mcp_servers.ix-memory]` at `command args`, as `codex mcp add` would. + + The fallback for a CLI whose `ix mcp install` has no `--force`. Edits a copy, + and swaps it in only if the copy parses and differs from the original in + exactly that one entry; anything else is left for the user. Returns whether + the file was rewritten. + """ + if tomllib is None: + return False + try: + text = config_path.read_text(encoding="utf-8-sig") + before = tomllib.loads(text) + except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): + return False + + lines = text.splitlines(keepends=True) + start = next((i for i, line in enumerate(lines) if _MCP_TABLE_RE.match(line)), None) + if start is None: + return False + end = start + 1 + while end < len(lines) and not _ANY_TABLE_RE.match(lines[end]): + end += 1 + body = [ + "[mcp_servers.ix-memory]\n", + f"command = {json.dumps(command)}\n", + f"args = {json.dumps(args)}\n", + ] + if end < len(lines): + body.append("\n") + updated = "".join(lines[:start] + body + lines[end:]) + + try: + after = tomllib.loads(updated) + except tomllib.TOMLDecodeError: + return False + entry = after.get("mcp_servers", {}).get(PLUGIN_NAME, {}) + if entry.get("command") != command or entry.get("args") != args: + return False + for parsed in (before, after): + parsed.get("mcp_servers", {}).pop(PLUGIN_NAME, None) + if before != after: + return False + + tx = Transaction() + tx.write(config_path, updated.encode("utf-8")) + tx.commit() + return True + + +# -------------------------------------------------------------------------- +# Hooks +# -------------------------------------------------------------------------- + + def _source_git_commit() -> str | None: try: result = subprocess.run( @@ -313,66 +719,133 @@ def _source_git_commit() -> str | None: return None -def write_version_file(target_root: Path) -> Path: - """Write .codex/ix-plugin-version.json into the install target.""" - meta = _read_plugin_json() - plugin_version = meta.get("version", "unknown") - plugin_name = meta.get("name", PLUGIN_NAME) +def _installed_manifest(target_root: Path) -> dict: + path = target_root / VERSION_FILE + try: + payload = json.loads(path.read_text(encoding="utf-8-sig")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError): + return {} + return payload if isinstance(payload, dict) else {} + +def plan_version_file(tx: Transaction, target_root: Path, files: list[str]) -> Path: + """Plan `.codex/ix-plugin-version.json`: what is installed, and which files are the plugin's. + + Left alone when only the timestamp would change, so a reinstall that changes + nothing writes nothing. + """ + meta = _read_plugin_json() payload: dict = { - "plugin_name": plugin_name, - "plugin_version": plugin_version, + "plugin_name": meta.get("name", PLUGIN_NAME), + "plugin_version": meta.get("version", "unknown"), "source_path": str(repo_root()), - "installed_at": datetime.datetime.now(datetime.timezone.utc).isoformat(timespec="seconds"), + "installed_at": datetime.datetime.now(datetime.timezone.utc).isoformat( + timespec="seconds" + ), } commit = _source_git_commit() if commit: payload["git_commit"] = commit - - version_path = target_root / ".codex" / "ix-plugin-version.json" - version_path.parent.mkdir(parents=True, exist_ok=True) - with version_path.open("w") as fh: - json.dump(payload, fh, indent=2) - fh.write("\n") + payload["files"] = sorted(files) + + version_path = target_root / VERSION_FILE + current = _installed_manifest(target_root) + if {k: v for k, v in current.items() if k != "installed_at"} != { + k: v for k, v in payload.items() if k != "installed_at" + } or tx.ops: + tx.write(version_path, _json_bytes(payload)) return version_path -def shell_free_hook_command(python: str, launcher: Path, hook_name: str) -> str: - """The `hooks.json` command for `hook_name`, with no shell in it. - - Both paths are quoted: #349 is a live report from a user whose profile is - `C:\\Users\\Win 10`, so a space in either path is a case that actually - happens rather than a hypothetical. - """ - return f'"{python}" "{launcher}" {hook_name}' - - HOOK_COMMAND_RE = re.compile( - # The shipped command is `/bin/sh -lc '... .codex/hooks/.py ...'`. The + # The shipped command is `/bin/sh -c '... .codex/hooks/.py ...'`. The # hook's own name is the only part that varies between the five entries, so # that is what gets recovered here. Anchored on the hooks directory so a # future command shape that still names the file keeps working. r"\.codex[/\\]hooks[/\\](?P[A-Za-z_][A-Za-z0-9_]*)\.py" ) +# Every hook script any version has registered in hooks.json, plus the Windows +# launcher 2.4.x pointed commands at. A handler whose command names one of these +# is the plugin's, whatever else about it changed between versions: `-lc` became +# `-c`, `Bash` became `Bash|apply_patch`, Windows got `_launch.py`. +PLUGIN_HOOK_SCRIPTS = frozenset( + {"session_start", "user_prompt_submit", "pre_tool_use", "post_tool_use", "stop", "_launch"} +) -def render_hooks_json(source: Path, launcher: Path) -> str | None: - """The hooks.json this machine should have, or None to install the source as-is. +_PS_QUOTES = "'\u2018\u2019\u201a\u201b" - The rewrite has to be expressed as *desired content* rather than as an edit - applied afterwards. Editing after install_file makes the installed file - permanently differ from the source, so the next run's content comparison - fails and raises FileExistsError -- and the documented Windows one-liner - (`irm ... | iex`) never passes --force, so every update would die, after - install_plugin had already written and before install_mcp ran. Comparing - against what this platform is supposed to end up with is idempotent by - construction. - utf-8-sig, not utf-8: PowerShell 5.1's Set-Content writes a BOM by default, - and json.loads rejects one. Reading it as plain utf-8 meant a BOM'd file - raised, the rewrite was skipped, and the installer reported success with all - five hooks still dead -- the exact failure this exists to remove. The same - trap, and the same fix, as hooks_disabled_by above. +def powershell_literal(text: str) -> str: + """`text` as a PowerShell single-quoted literal: no `$` expansion, no escapes. + + PowerShell treats the typographic single quotes as quote characters too, so + each of them is doubled along with the ASCII one. + """ + return "'" + "".join(ch * 2 if ch in _PS_QUOTES else ch for ch in text) + "'" + + +def windows_hook_command(python: str, launcher: object, hook_name: str) -> str: + """The `commandWindows` that launches `hook_name` without /bin/sh. + + Codex 0.155 runs a hook command through the session's shell + (codex-rs/core/src/session/mod.rs, hooks config: `shell.derive_exec_args`), + and on Windows that shell is PowerShell (shell-command/src/shell_detect.rs, + `default_user_shell`): `powershell.exe -NoProfile -Command `. There + a command that starts with a quoted path is a string expression, so the old + `"python" "launcher" name` was a parse error before any hook ran. The call + operator `&` makes it an invocation; single quotes keep a `$` in a path from + being expanded. + + Both paths are quoted: #349 is a live report from a user whose profile is + `C:\\Users\\Win 10`. + """ + return f"& {powershell_literal(python)} {powershell_literal(str(launcher))} {hook_name}" + + +def _plugin_script(command: object) -> str | None: + if not isinstance(command, str): + return None + for match in HOOK_COMMAND_RE.finditer(command): + if match.group("name") in PLUGIN_HOOK_SCRIPTS: + return match.group("name") + return None + + +def is_plugin_handler(handler: object) -> bool: + if not isinstance(handler, dict): + return False + return any( + _plugin_script(handler.get(key)) is not None for key in ("command", "commandWindows") + ) + + +def _add_windows_commands(payload: dict, python: str, launcher: object) -> bool: + """Give every plugin handler a `commandWindows`. Returns whether anything changed. + + `command` keeps the POSIX form, so a repo-local hooks.json still works for + a teammate on macOS or Linux; Codex picks `commandWindows` on Windows + (hooks/src/engine/discovery.rs). + """ + changed = False + for blocks in payload["hooks"].values(): + for block in blocks: + for hook in block.get("hooks", []): + match = HOOK_COMMAND_RE.search(hook.get("command", "")) + if not match or match.group("name") == "_launch": + continue + desired = windows_hook_command(python, launcher, match.group("name")) + if hook.get("commandWindows") != desired: + hook["commandWindows"] = desired + changed = True + return changed + + +def render_hooks_json(source: Path, launcher: object) -> str | None: + """The plugin's hooks.json for this machine, or None to use the source as-is. + + Off Windows the shipped file is already right. On Windows each handler gains + a `commandWindows` naming this interpreter and the installed launcher. """ if os.name != "nt": return None @@ -380,15 +853,11 @@ def render_hooks_json(source: Path, launcher: Path) -> str | None: try: payload = json.loads(source.read_text(encoding="utf-8-sig")) except (OSError, json.JSONDecodeError, UnicodeDecodeError): - # A hooks.json we cannot parse is not one we should rewrite. Leave it - # for the user to see rather than replacing it with a guess. return None try: - changed = _rewrite_hook_commands(payload, launcher) + changed = _add_windows_commands(payload, sys.executable, launcher) except (AttributeError, KeyError, TypeError): - # Parsed, but not the shape we understand. Same reasoning as above: this - # used to escape the JSON guard and abort the installer with a traceback. return None if not changed: @@ -396,102 +865,267 @@ def render_hooks_json(source: Path, launcher: Path) -> str | None: return json.dumps(payload, indent=2) + "\n" -def _rewrite_hook_commands(payload: dict, launcher: Path) -> bool: - changed = False - for blocks in payload["hooks"].values(): - for block in blocks: - for hook in block.get("hooks", []): - command = hook.get("command", "") - if "/bin/sh" not in command: - continue # already rewritten, or never was a shell command - match = HOOK_COMMAND_RE.search(command) - if not match: - continue - hook["command"] = shell_free_hook_command( - sys.executable, launcher, match.group("name") - ) - changed = True - return changed +def merge_hooks(existing: dict | None, ours: dict) -> dict: + """`existing` with every plugin handler replaced by `ours`, everything else kept. + The plugin's groups go where its first old one was, so the other hooks keep + their positions -- Codex keys a hook's trust by event, group and handler + index (hooks/src/engine/discovery.rs, `hook_key`), and shifting a user's hook + would make Codex ask them to re-trust it. + """ + if existing is None: + return copy.deepcopy(ours) + + result = copy.deepcopy(existing) + events = result.setdefault("hooks", {}) + ours_events = ours.get("hooks", {}) + + for event in list(events): + groups = events[event] + kept: list = [] + insert_at: int | None = None + for group in groups: + handlers = group.get("hooks", []) + theirs = [h for h in handlers if not is_plugin_handler(h)] + if len(theirs) == len(handlers): + kept.append(group) + continue + if insert_at is None: + insert_at = len(kept) + if theirs: + kept.append({**group, "hooks": theirs}) + new_groups = ours_events.get(event, []) + if insert_at is None and not new_groups: + continue # nothing of the plugin's here, before or after + if insert_at is None: + insert_at = len(kept) + kept[insert_at:insert_at] = copy.deepcopy(new_groups) + if kept: + events[event] = kept + else: + del events[event] + for event, groups in ours_events.items(): + if event not in events: + events[event] = copy.deepcopy(groups) + return result -def install_hooks(target_root: Path, mode: str, force: bool) -> list[Path]: - installed: list[Path] = [] +def _hooks_shape_ok(payload: object) -> bool: + if not isinstance(payload, dict): + return False + events = payload.get("hooks", {}) + if not isinstance(events, dict): + return False + for groups in events.values(): + if not isinstance(groups, list): + return False + for group in groups: + if not isinstance(group, dict) or not isinstance(group.get("hooks", []), list): + return False + return True + + +def plan_hooks(tx: Transaction, target_root: Path, mode: str, force: bool) -> list[Path]: + paths: list[Path] = [] codex_dir = target_root / ".codex" hooks_source = source_codex_dir() / "hooks" hooks_destination = codex_dir / "hooks" hooks_json = codex_dir / "hooks.json" hooks_json_source = source_codex_dir() / "hooks.json" - rendered = render_hooks_json(hooks_json_source, hooks_destination / "_launch.py") - if rendered is not None and _same_path(hooks_json, hooks_json_source): - # Installing the checkout into itself. The rendered file names this - # machine's interpreter, so writing it here would leave the tracked - # source modified and every later diff carrying one developer's paths. - rendered = None - if rendered is None: - install_file(source_codex_dir() / "hooks.json", hooks_json, mode, force) - else: - # Windows: the file this machine needs is not the source, so it cannot be - # symlinked to it either -- the rendered command names this interpreter's - # absolute path. Compare against the rendered content so a second run is - # a no-op rather than a FileExistsError. - install_rendered(hooks_json, rendered, force) - installed.append(hooks_json) - - hooks_destination.mkdir(parents=True, exist_ok=True) + source_bytes = hooks_json_source.read_bytes() + + if not _same_location(hooks_json, hooks_json_source): + # Installing the checkout into itself is skipped: the Windows form names + # this machine's interpreter, and writing it would leave the tracked + # source carrying one developer's paths. + rendered = render_hooks_json(hooks_json_source, hooks_destination / "_launch.py") + ours_text = rendered if rendered is not None else source_bytes.decode("utf-8") + ours = json.loads(ours_text) + + existing: dict | None = None + if _lexists(hooks_json): + try: + existing = json.loads(hooks_json.read_text(encoding="utf-8-sig")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + if not force: + raise InstallError( + f"{hooks_json} is not valid JSON ({exc}), so the Ix hooks cannot be " + "merged into it. Fix it, or re-run with --force to replace it." + ) from exc + existing = None + else: + if not _hooks_shape_ok(existing): + if not force: + raise InstallError( + f"{hooks_json} is not in the shape Codex reads " + '({"hooks": {"": [{"hooks": [...]}]}}). ' + "Fix it, or re-run with --force to replace it." + ) + existing = None + + merged = merge_hooks(existing, ours) + if merged == ours and mode == "symlink" and rendered is None: + tx.link(hooks_json, hooks_json_source) + elif merged == ours: + tx.write(hooks_json, ours_text.encode("utf-8")) + elif existing is None or merged != existing or hooks_json.is_symlink(): + tx.write(hooks_json, _json_bytes(merged)) + paths.append(hooks_json) + + shipped: list[str] = [] for source in sorted(hooks_source.glob("*.py")): destination = hooks_destination / source.name - install_file(source, destination, mode, force) - installed.append(destination) + relative = destination.relative_to(target_root).as_posix() + shipped.append(relative) + if not _same_location(destination, source): + tx.install(destination, source, mode) + paths.append(destination) + + # Files an earlier version installed that this one does not ship. + previous = _installed_manifest(target_root).get("files") + owned = previous if isinstance(previous, list) else list(LEGACY_HOOK_FILES) + for relative in owned: + if not isinstance(relative, str) or relative in shipped: + continue + if not re.fullmatch(r"\.codex/hooks/[A-Za-z0-9_]+\.py", relative): + continue # never act on a path outside the hooks directory + tx.remove(target_root / relative) - installed.append(write_version_file(target_root)) + paths.append(plan_version_file(tx, target_root, shipped)) + return paths - return installed +def install_hooks(target_root: Path, mode: str, force: bool) -> list[Path]: + tx = Transaction() + paths = plan_hooks(tx, target_root, mode, force) + tx.commit() + return paths -def install_plugin( - target_root: Path, - home_install: bool, - mode: str, - force: bool, - marketplace_name: str, - marketplace_display_name: str, -) -> list[Path]: - installed: list[Path] = [] - if home_install: - plugin_destination = target_root / ".codex" / "plugins" / PLUGIN_NAME - plugin_path = f"./.codex/plugins/{PLUGIN_NAME}" - else: - plugin_destination = target_root / "plugins" / PLUGIN_NAME - plugin_path = f"./plugins/{PLUGIN_NAME}" +# -------------------------------------------------------------------------- +# Leftovers from earlier versions +# -------------------------------------------------------------------------- - install_tree(source_plugin_dir(), plugin_destination, mode, force) - installed.append(plugin_destination) +_OLD_CACHE_FILE_RE = re.compile( + r"ix-(status|pro|runtime-health)\.json" + r"|ix-briefing(-[0-9a-f]{16})?\.txt" + r"|auto-map-[0-9a-f]{16}(\.[A-Za-z]+)?" +) - plugin_meta = _read_plugin_json() - plugin_version = str(plugin_meta.get("version", "")) - plugin_description = str(plugin_meta.get("description", "")) - marketplace_path = target_root / ".agents" / "plugins" / "marketplace.json" - update_marketplace( - marketplace_path, - plugin_path, - marketplace_name, - marketplace_display_name, - force, - version=plugin_version, - description=plugin_description, - ) - installed.append(marketplace_path) +def legacy_cache_dirs() -> list[Path]: + """Where hooks before 2.4.2 kept their cache: `$TMPDIR/ix-codex-hooks`, else /tmp.""" + candidates = [ + Path(os.environ.get("TMPDIR") or "/tmp") / "ix-codex-hooks", + Path(tempfile.gettempdir()) / "ix-codex-hooks", + ] + unique: list[Path] = [] + for path in candidates: + if path not in unique: + unique.append(path) + return unique + - return installed +def remove_legacy_cache_dir(path: Path) -> bool: + """Delete an old shared cache dir, only if it is this user's and holds only cache files. + + It lived in a world-writable directory, so its name alone proves nothing: a + directory someone else created, a symlink, or one with anything unexpected + in it is left exactly as it is. + """ + try: + info = os.lstat(path) + except OSError: + return False + if not stat.S_ISDIR(info.st_mode): + return False + if hasattr(os, "getuid") and info.st_uid != os.getuid(): + return False + try: + entries = list(os.scandir(path)) + except OSError: + return False + for entry in entries: + if not entry.is_file(follow_symlinks=False) or not _OLD_CACHE_FILE_RE.fullmatch(entry.name): + return False + try: + for entry in entries: + os.unlink(entry.path) + os.rmdir(path) + except OSError: + return False + return True +def remove_legacy_mcp_files(target_root: Path, config_path: Path) -> list[Path]: + """Delete the MCP server 2.4.1 and earlier copied in, once nothing launches it. + + Not part of the transaction: it has to wait for the registration step, and a + Codex config that still points at the file -- because `ix` was missing, or + the user declined to replace a conflicting entry -- keeps it in place. + """ + try: + config_text = config_path.read_text(encoding="utf-8-sig") + except (OSError, UnicodeDecodeError): + config_text = "" + # Deliberately coarse: any registration still naming an `mcp/server.py` + # keeps these. A path compared exactly would miss the same file reached + # through a symlinked home, and deleting a server Codex still launches is + # the one outcome worse than leaving two stale files behind. + if "mcp/server.py" in config_text.replace("\\\\", "/").replace("\\", "/"): + return [] + + removed: list[Path] = [] + for relative in LEGACY_MCP_FILES: + path = target_root / relative + if not path.is_file() or path.is_symlink(): + continue + try: + path.unlink() + removed.append(path) + except OSError: + continue + mcp_dir = target_root / ".codex" / "mcp" + cache = mcp_dir / "__pycache__" + if removed and cache.is_dir() and not cache.is_symlink(): + stale = [p for p in cache.iterdir() if re.fullmatch(r"(server|ix_llm)\..*\.pyc", p.name)] + if len(stale) == len(list(cache.iterdir())): + for pyc in stale: + pyc.unlink(missing_ok=True) + try: + cache.rmdir() + except OSError: + pass + try: + if removed: + mcp_dir.rmdir() # only succeeds if empty + except OSError: + pass + return removed + + +# -------------------------------------------------------------------------- +# MCP registration +# -------------------------------------------------------------------------- + MIN_IX_VERSION_FOR_MCP = (0, 9, 3) +# `ix mcp install --host codex` asks `codex mcp list` (20s cap inside the CLI) +# and then runs `codex mcp add`. This bounds the whole thing, so a wedged host +# CLI cannot hang the installer. +MCP_INSTALL_TIMEOUT_SECONDS = 120 + + +def _mcp_timeout() -> float: + raw = os.environ.get("IX_CODEX_MCP_TIMEOUT", "") + try: + value = float(raw) + except ValueError: + return MCP_INSTALL_TIMEOUT_SECONDS + return value if value > 0 else MCP_INSTALL_TIMEOUT_SECONDS + def _ix_executable() -> str | None: """The `ix` on PATH, as an absolute path, or None if there is none. @@ -522,7 +1156,7 @@ def _ix_version(executable: str) -> tuple[int, ...] | None: ).stdout except (OSError, ValueError, subprocess.SubprocessError): return None - match = re.search(r"(\d+)\.(\d+)\.(\d+)", out) + match = re.search(r"(\d+)\.(\d+)\.(\d+)", out or "") return tuple(int(g) for g in match.groups()) if match else None @@ -538,31 +1172,151 @@ def install_mcp(target_root: Path, mode: str, force: bool) -> list[Path]: return [] -def main() -> None: - args = parse_args() +class McpResult: + def __init__(self, ok: bool, message: str) -> None: + self.ok = ok + self.message = message + + +def _first_lines(text: str | None, limit: int = 5) -> str: + lines = [line.rstrip() for line in (text or "").splitlines() if line.strip()] + return "\n".join(f" {line}" for line in lines[:limit]) + + +def _codex_outcome(stdout: str | None) -> dict | None: + """The `codex` row of `ix mcp install --format json`, if the output was that.""" + text = (stdout or "").strip() + start = text.find("{") + if start == -1: + return None + try: + report, _ = json.JSONDecoder().raw_decode(text[start:]) + except json.JSONDecodeError: + return None + hosts = report.get("hosts") if isinstance(report, dict) else None + for host in hosts if isinstance(hosts, list) else []: + if isinstance(host, dict) and host.get("id") == "codex": + return host + return None + + +def register_mcp(executable: str, force: bool) -> McpResult: + """Run `ix mcp install --host codex`, bounded, and say plainly how it went.""" + config_path = codex_config_path() + stale = stale_mcp_registration(config_path) + argv = [executable, "mcp", "install", "--host", "codex", "--format", "json"] + if stale or force: + # A registration of the deleted server.py is ours to replace; `ix mcp + # install` cannot tell it from someone else's server and needs --force. + argv.append("--force") + + timeout = _mcp_timeout() + try: + result = subprocess.run(argv, capture_output=True, text=True, timeout=timeout) + except subprocess.TimeoutExpired: + return McpResult( + False, + f"`ix mcp install --host codex` did not finish within {timeout:g}s and was stopped. " + "Run it yourself once Codex is responsive.", + ) + except (OSError, ValueError) as exc: + return McpResult(False, f"could not run `{executable} mcp install`: {exc}") + + if result.returncode != 0 and "--force" in argv and "--force" in (result.stderr or "") \ + and "unknown option" in (result.stderr or ""): + # A CLI without the flag. Do what `codex mcp add` would have done to the + # entry -- only possible for the stale registration recognised above. + if stale: + command = executable if os.name == "nt" else "ix" + if rewrite_mcp_registration(config_path, command, ["mcp"]): + return McpResult(True, f"Replaced the old ix-memory server ({stale}) with `ix mcp`.") + return McpResult( + False, + "this `ix mcp install` has no --force, so the old ix-memory registration " + f"in {config_path} could not be replaced. Run `codex mcp remove ix-memory`, " + "then re-run the installer.", + ) + + host = _codex_outcome(result.stdout) + outcome = host.get("outcome") if host else None + if result.returncode != 0 or outcome == "failed": + detail = _first_lines(result.stderr) or _first_lines(result.stdout) + note = f" ({host.get('note')})" if host and host.get("note") else "" + return McpResult( + False, + f"`ix mcp install --host codex` failed (exit {result.returncode}){note}." + + (f"\n{detail}" if detail else ""), + ) + if outcome in ("registered", "already-registered"): + verb = "Registered" if outcome == "registered" else "Already registered:" + replaced = f" (replaced the old {stale})" if stale and outcome == "registered" else "" + return McpResult(True, f"{verb} `ix mcp` as the ix-memory MCP server in Codex{replaced}.") + if outcome == "conflict": + return McpResult( + False, + f"{config_path} already has an ix-memory MCP server that is not `ix mcp`; it was " + "left alone. Re-run with --force to replace it.", + ) + if outcome == "not-installed": + return McpResult( + False, + "the Codex CLI (`codex`) is not on PATH, so nothing was registered. Run " + "`ix mcp install --host codex` once it is.", + ) + if outcome is None: + # A CLI that printed something other than the JSON asked for. Its exit + # status is all there is to go on. + return McpResult(True, "Ran `ix mcp install --host codex`.") + return McpResult(False, f"`ix mcp install --host codex` reported '{outcome}'.") + + +# -------------------------------------------------------------------------- +# main +# -------------------------------------------------------------------------- + + +def warn(message: str) -> None: + print(f" [!!] {message}") + + +def main(argv: list[str] | None = None) -> int: + args = parse_args(argv) target_root = target_root_from_args(args) target_root.mkdir(parents=True, exist_ok=True) + tx = Transaction() installed: list[Path] = [] - if args.plugin: - installed.extend( - install_plugin( - target_root, - args.home, - args.mode, - args.force, - args.marketplace_name, - args.marketplace_display_name, + hooks_json = target_root / ".codex" / "hooks.json" + try: + if args.plugin: + installed.extend( + plan_plugin( + tx, + target_root, + args.home, + args.mode, + args.force, + args.marketplace_name, + args.marketplace_display_name, + ) ) - ) - if args.hooks: - installed.extend(install_hooks(target_root, args.mode, args.force)) - if args.mcp: - installed.extend(install_mcp(target_root, args.mode, args.force)) + if args.hooks: + installed.extend(plan_hooks(tx, target_root, args.mode, args.force)) + hooks_changed = tx.changes(hooks_json) + changed = tx.commit() + except InstallError as exc: + print(f" [error] {exc}", file=sys.stderr) + return 1 print(f"Installed into: {target_root}") for path in installed: print(path) + if not changed: + print("Everything was already up to date.") + else: + for path in changed: + if not _lexists(path): + print(f"Removed (no longer shipped): {path}") if args.plugin: print("Plugin files and marketplace entry were installed.") @@ -571,35 +1325,52 @@ def main() -> None: print("After that, type '$ix-tutorial' manually in chat.") print("Local Codex plugins do not reliably expose skill autocomplete or slash popups.") if args.hooks: - print("Restart Codex so it reloads hooks.json.") - # Codex runs a non-managed hook only once its exact definition has been - # reviewed and trusted, and re-asks whenever that definition changes - # (codex-rs/hooks/src/engine/discovery.rs, hook_trust_status). - print("Codex will ask you to review and trust the Ix hooks on its next start;") - print("until you do, it skips them. `codex exec` cannot ask, so it skips them too.") - disabled = hooks_disabled_by(target_root / ".codex" / "config.toml") + if hooks_changed: + print("Restart Codex so it reloads hooks.json.") + # Codex runs a non-managed hook only once its exact definition has + # been reviewed and trusted, and re-asks whenever that definition + # changes (codex-rs/hooks/src/engine/discovery.rs, hook_trust_status). + print("Codex will ask you to review and trust the Ix hooks on its next start;") + print("until you do, it skips them. `codex exec` cannot ask, so it skips them too.") + else: + print("hooks.json is unchanged, so hooks you already trusted stay trusted.") + config_toml = target_root / ".codex" / "config.toml" + disabled = hooks_disabled_by(config_toml) if disabled is not None: print(f"Hooks are turned off in .codex/config.toml ([features] {disabled} = false).") print("Remove that line, or set `hooks = true`, for the Ix hooks to run.") + elif legacy_hooks_flag(config_toml): + print("`codex_hooks = true` in .codex/config.toml was written by an older installer.") + print("Codex now enables hooks by default and warns about that flag; you can delete it.") + for cache in legacy_cache_dirs(): + if remove_legacy_cache_dir(cache): + print(f"Removed the old hook cache {cache}.") + if args.mcp: executable = _ix_executable() version = _ix_version(executable) if executable is not None else None if version is None: - print("Could not run `ix --version`; install the Ix CLI, then re-run with --mcp.") + warn("Could not run `ix --version`; install the Ix CLI, then re-run with --mcp.") elif version < MIN_IX_VERSION_FOR_MCP: wanted = ".".join(str(part) for part in MIN_IX_VERSION_FOR_MCP) got = ".".join(str(part) for part in version) - print(f"The MCP server needs Ix CLI >= {wanted} (found {got}). Run `ix upgrade`.") + warn(f"The MCP server needs Ix CLI >= {wanted} (found {got}). Run `ix upgrade`.") else: # `ix mcp install` owns the per-host detail — including resolving the - # launcher for the seven hosts it registers, on the Windows where npm - # ships ix.CMD and no ix.exe. Reaching it has the same problem one - # level up, so the resolved path is what is spawned: the bare name - # would raise FileNotFoundError before `check=False` had any say, and - # the registration this flag exists to write would never happen. + # launcher on the Windows where npm ships ix.CMD and no ix.exe. + # Reaching it has the same problem one level up, so the resolved path + # is what is spawned. print("Registering the Ix MCP server with Codex:") - subprocess.run([executable, "mcp", "install", "--host", "codex"], check=False) + outcome = register_mcp(executable, args.force) + if outcome.ok: + print(f" {outcome.message}") + else: + warn(f"MCP registration: {outcome.message}") + for path in remove_legacy_mcp_files(target_root, codex_config_path()): + print(f"Removed the old MCP server file {path}.") + + return 0 if __name__ == "__main__": - main() + sys.exit(main()) diff --git a/test-local.sh b/test-local.sh index f6f9863..9aa73e6 100755 --- a/test-local.sh +++ b/test-local.sh @@ -88,6 +88,14 @@ else printf '%s\n' "$map_out" | sed 's/^/ /' fi +# Every installer run there is against a temp HOME/CODEX_HOME and a fake `ix`. +if upgrade_out=$(python3 "$REPO/tests/test_installer_upgrade.py" 2>&1); then + ok "re-running the installer upgrades any earlier install, merges hooks, never half-writes" +else + fail "installer upgrade tests failed" + printf '%s\n' "$upgrade_out" | sed 's/^/ /' +fi + if argv_out=$(python3 "$REPO/tests/test_ix_argv_resolution.py" 2>&1); then ok "ix invocations resolve through PATHEXT and refuse cmd metacharacters" else diff --git a/tests/test_installer_upgrade.py b/tests/test_installer_upgrade.py new file mode 100644 index 0000000..b48d35d --- /dev/null +++ b/tests/test_installer_upgrade.py @@ -0,0 +1,569 @@ +#!/usr/bin/env python3 +# Copyright 2026 Ix Infrastructure Inc. + +"""Re-running the installer is how users upgrade, so it has to just work. + +Up to 2.4.3 a re-run after any change -- a new version, an edited hook, a hook +of the user's own in hooks.json -- raised FileExistsError, and the hosted +one-liners never pass --force. By then part of the install had been written. +Even with --force, an `ix-memory` registration of the server.py that 2.4.1 and +earlier shipped was never replaced, because `ix mcp install` ran without +--force, unbounded, with its exit status ignored. + +Every run here is the real installer in a subprocess, against a temporary HOME +and CODEX_HOME, with a fake `ix` first on PATH that records its argv and +emulates `ix mcp install --host codex` against the temp config. Nothing here +reads or writes the real ~/.codex. +""" + +from __future__ import annotations + +import importlib.util +import json +import os +import re +import shutil +import subprocess +import sys +import tempfile +import textwrap +import unittest +from pathlib import Path +from unittest.mock import patch + +REPO = Path(__file__).resolve().parents[1] +INSTALLER = REPO / "scripts" / "install_codex_integration.py" +SHIPPED_HOOKS_JSON = REPO / ".codex" / "hooks.json" +PLUGIN_VERSION = json.loads( + (REPO / "plugins" / "ix-memory" / ".codex-plugin" / "plugin.json").read_text(encoding="utf-8") +)["version"] + +try: + import tomllib +except ModuleNotFoundError: # pragma: no cover - 3.10 + tomllib = None # type: ignore[assignment] + + +# The fake CLI. `--version` answers; `mcp install --host codex` does what the +# real one does to Codex's config: an `ix-memory` entry that is not `ix mcp` is +# a conflict, left alone, unless --force. FAKE_IX_MODE selects a failure. +FAKE_IX = textwrap.dedent( + r''' + import json, os, re, sys, time + from pathlib import Path + + argv = sys.argv[1:] + with open(os.environ["IX_FAKE_LOG"], "a", encoding="utf-8") as log: + log.write(json.dumps(argv) + "\n") + mode = os.environ.get("FAKE_IX_MODE", "ok") + + if argv[:1] == ["--version"]: + print(os.environ.get("FAKE_IX_VERSION", "0.11.1")) + sys.exit(0) + if argv[:2] != ["mcp", "install"]: + sys.exit(0) + + def report(outcome, code=0): + print(json.dumps({"hosts": [{"id": "codex", "label": "Codex CLI", + "outcome": outcome}], + "registered": int(outcome == "registered"), "conflicts": 0})) + sys.exit(code) + + if mode == "hang": + time.sleep(6) + report("registered") + if mode == "fail": + print("codex mcp add exploded", file=sys.stderr) + report("failed", 1) + if mode == "noforce" and "--force" in argv: + print("error: unknown option '--force'", file=sys.stderr) + sys.exit(1) + + config = Path(os.environ["CODEX_HOME"]) / "config.toml" + text = config.read_text(encoding="utf-8") if config.exists() else "" + table = re.compile(r"^\[mcp_servers\.ix-memory\]\n(?:(?!\[).*\n?)*", re.M) + found = table.search(text) + if found and 'command = "ix"' in found.group(0) and '"mcp"' in found.group(0): + report("already-registered") + if found and "--force" not in argv: + report("conflict") + text = table.sub("", text) + if text and not text.endswith("\n"): + text += "\n" + text += '[mcp_servers.ix-memory]\ncommand = "ix"\nargs = ["mcp"]\n' + config.parent.mkdir(parents=True, exist_ok=True) + config.write_text(text, encoding="utf-8") + report("registered") + ''' +) + + +def snapshot(root: Path) -> dict[str, object]: + """Every path under `root` with its bytes, link target, or 'dir'.""" + state: dict[str, object] = {} + for path in sorted(root.rglob("*")): + key = path.relative_to(root).as_posix() + if path.is_symlink(): + state[key] = ("link", os.readlink(path)) + elif path.is_dir(): + state[key] = "dir" + else: + state[key] = path.read_bytes() + return state + + +def old_hooks_json(windows_launcher: str | None = None) -> str: + """hooks.json as 2.4.2 and earlier shipped it: `-lc`, and `Bash`-only matchers. + + With `windows_launcher`, as the 2.4.x installer rendered it on Windows. + """ + payload = json.loads(SHIPPED_HOOKS_JSON.read_text(encoding="utf-8")) + for blocks in payload["hooks"].values(): + for block in blocks: + if block.get("matcher") == "Bash|apply_patch": + block["matcher"] = "Bash" + for hook in block["hooks"]: + hook["command"] = hook["command"].replace("/bin/sh -c ", "/bin/sh -lc ", 1) + if windows_launcher: + name = re.search(r"hooks/(\w+)\.py", hook["command"]).group(1) + hook["command"] = f'"C:\\Python311\\python.exe" "{windows_launcher}" {name}' + return json.dumps(payload, indent=2) + "\n" + + +def plugin_handlers(payload: dict) -> list[dict]: + return [ + hook + for blocks in payload["hooks"].values() + for block in blocks + for hook in block["hooks"] + if re.search(r"\.codex[/\\]hooks[/\\]\w+\.py", hook.get("command", "")) + ] + + +USER_HOOK = { + "type": "command", + "command": "/usr/local/bin/my-audit-hook --event pre", + "statusMessage": "my own hook", +} + + +class InstallerHarness(unittest.TestCase): + def setUp(self) -> None: + self._dir = tempfile.TemporaryDirectory() + self.addCleanup(self._dir.cleanup) + self.root = Path(self._dir.name).resolve() + self.home = self.root / "home" + self.codex = self.home / ".codex" + self.codex.mkdir(parents=True) + self.tmp = self.root / "tmp" + self.tmp.mkdir() + self.bin = self.root / "bin" + self.bin.mkdir() + self.log = self.root / "ix.log" + self.log.write_text("", encoding="utf-8") + script = self.root / "fake_ix.py" + script.write_text(FAKE_IX, encoding="utf-8") + if os.name == "nt": + (self.bin / "ix.cmd").write_text( + f'@"{sys.executable}" "{script}" %*\r\n', encoding="utf-8" + ) + else: + fake = self.bin / "ix" + fake.write_text(f'#!/bin/sh\nexec "{sys.executable}" "{script}" "$@"\n', encoding="utf-8") + fake.chmod(0o755) + + def env(self, ix: bool = True, **extra: str) -> dict[str, str]: + # Drop every PATH entry that has a real `ix`, so only the fake answers. + path = [ + entry + for entry in os.environ.get("PATH", "").split(os.pathsep) + if entry and shutil.which("ix", path=entry) is None + ] + if ix: + path.insert(0, str(self.bin)) + env = dict(os.environ) + env.update( + { + "HOME": str(self.home), + "USERPROFILE": str(self.home), + "CODEX_HOME": str(self.codex), + "TMPDIR": str(self.tmp), + "TMP": str(self.tmp), + "TEMP": str(self.tmp), + "XDG_STATE_HOME": str(self.root / "state"), + "PATH": os.pathsep.join(path), + "IX_FAKE_LOG": str(self.log), + } + ) + env.pop("FAKE_IX_MODE", None) + env.update(extra) + return env + + def install(self, *args: str, ix: bool = True, timeout: float = 60, **extra: str): + argv = list(args) or ["--home"] + return subprocess.run( + [sys.executable, str(INSTALLER), *argv], + capture_output=True, + text=True, + env=self.env(ix=ix, **extra), + timeout=timeout, + ) + + def ok(self, *args: str, **kwargs): + result = self.install(*args, **kwargs) + self.assertEqual(0, result.returncode, result.stdout + result.stderr) + self.assertNotIn("Traceback", result.stderr) + return result + + def ix_calls(self) -> list[list[str]]: + return [json.loads(line) for line in self.log.read_text(encoding="utf-8").splitlines()] + + def mcp_calls(self) -> list[list[str]]: + return [call for call in self.ix_calls() if call[:2] == ["mcp", "install"]] + + def hooks_json(self) -> dict: + return json.loads((self.codex / "hooks.json").read_text(encoding="utf-8-sig")) + + def config(self) -> str: + path = self.codex / "config.toml" + return path.read_text(encoding="utf-8") if path.exists() else "" + + # -- a synthesized install from an earlier version --------------------- + + def synthesize(self, version: str, *, legacy_mcp: bool = False, windows: bool = False) -> None: + hooks = self.codex / "hooks" + hooks.mkdir(parents=True, exist_ok=True) + launcher = str(hooks / "_launch.py") if windows else None + if version == "2.4.3": + text = SHIPPED_HOOKS_JSON.read_text(encoding="utf-8") + else: + text = old_hooks_json(launcher) + (self.codex / "hooks.json").write_text(text, encoding="utf-8") + for source in (REPO / ".codex" / "hooks").glob("*.py"): + (hooks / source.name).write_text(f"# ix-memory {version} copy of {source.name}\n") + (self.codex / "ix-plugin-version.json").write_text( + json.dumps({"plugin_name": "ix-memory", "plugin_version": version, + "installed_at": "2026-01-01T00:00:00+00:00"}), + encoding="utf-8", + ) + if version != "2.4.3": + (self.codex / "config.toml").write_text( + 'model = "o3"\n\n[features]\ncodex_hooks = true\n', encoding="utf-8" + ) + plugin = self.codex / "plugins" / "ix-memory" + (plugin / ".codex-plugin").mkdir(parents=True) + (plugin / ".codex-plugin" / "plugin.json").write_text( + json.dumps({"name": "ix-memory", "version": version}), encoding="utf-8" + ) + # A skill an earlier version shipped and this one dropped. + (plugin / "skills" / "ix-search").mkdir(parents=True) + (plugin / "skills" / "ix-search" / "SKILL.md").write_text("old\n", encoding="utf-8") + market = self.home / ".agents" / "plugins" / "marketplace.json" + market.parent.mkdir(parents=True) + market.write_text(json.dumps({ + "name": "ix-codex-plugin", + "interface": {"displayName": "ix-codex-plugin"}, + "plugins": [ + {"name": "someone-else", "source": {"source": "local", "path": "./x"}}, + {"name": "ix-memory", "version": version, + "source": {"source": "local", "path": "./.codex/plugins/ix-memory"}}, + ], + }, indent=2), encoding="utf-8") + if legacy_mcp: + mcp = self.codex / "mcp" + mcp.mkdir() + (mcp / "server.py").write_text("# FastMCP server\n", encoding="utf-8") + (mcp / "ix_llm.py").write_text("# helper\n", encoding="utf-8") + server = (mcp / "server.py").as_posix() + with (self.codex / "config.toml").open("a", encoding="utf-8") as fh: + fh.write(f'\n[mcp_servers.ix-memory]\ncommand = "python3"\nargs = ["{server}"]\n') + + def assert_current_install(self, synthesized: bool = True) -> None: + hooks = self.codex / "hooks" + for source in (REPO / ".codex" / "hooks").glob("*.py"): + self.assertEqual(source.read_bytes(), (hooks / source.name).read_bytes(), source.name) + plugin = self.codex / "plugins" / "ix-memory" + self.assertEqual( + PLUGIN_VERSION, + json.loads((plugin / ".codex-plugin" / "plugin.json").read_text())["version"], + ) + self.assertFalse((plugin / "skills" / "ix-search").exists(), "a dropped skill survived") + handlers = plugin_handlers(self.hooks_json()) + self.assertEqual(5, len(handlers), "plugin hooks duplicated or missing") + for handler in handlers: + self.assertNotIn("-lc", handler["command"]) + matchers = [b.get("matcher") for b in self.hooks_json()["hooks"]["PreToolUse"]] + self.assertIn("Bash|apply_patch", matchers) + self.assertNotIn("Bash", matchers) + manifest = json.loads((self.codex / "ix-plugin-version.json").read_text()) + self.assertEqual(PLUGIN_VERSION, manifest["plugin_version"]) + self.assertIn(".codex/hooks/common.py", manifest["files"]) + market = json.loads((self.home / ".agents" / "plugins" / "marketplace.json").read_text()) + names = [p["name"] for p in market["plugins"]] + self.assertEqual(1, names.count("ix-memory")) + if synthesized: + self.assertIn("someone-else", names, "another plugin's entry was dropped") + entry = next(p for p in market["plugins"] if p["name"] == "ix-memory") + self.assertEqual(PLUGIN_VERSION, entry["version"]) + self.assertEqual([], list(self.home.rglob("*.ix-new-*")) + list(self.home.rglob("*.ix-old-*"))) + + +class FreshInstall(InstallerHarness): + def test_fresh_install(self) -> None: + result = self.ok() + if os.name == "nt": + for handler in plugin_handlers(self.hooks_json()): + self.assertTrue(handler["commandWindows"].startswith("& ")) + else: + self.assertEqual(SHIPPED_HOOKS_JSON.read_bytes(), (self.codex / "hooks.json").read_bytes()) + self.assertIn("trust the Ix hooks", result.stdout) + self.assertEqual( + [["mcp", "install", "--host", "codex", "--format", "json"]], self.mcp_calls() + ) + self.assertIn('command = "ix"', self.config()) + self.assertIn("Registered `ix mcp`", result.stdout) + + +class Reinstall(InstallerHarness): + def test_same_version_with_the_users_own_hook_added(self) -> None: + """What Codex users do between installs: add a hook of their own.""" + self.ok() + payload = self.hooks_json() + payload["hooks"].setdefault("PreToolUse", []).append({"matcher": "Bash", "hooks": [USER_HOOK]}) + (self.codex / "hooks.json").write_text(json.dumps(payload, indent=2), encoding="utf-8") + before = snapshot(self.home) + + result = self.ok() + self.assertEqual( + {k: v for k, v in before.items() if k != ".codex/config.toml"}, + {k: v for k, v in snapshot(self.home).items() if k != ".codex/config.toml"}, + "a same-version reinstall must change nothing", + ) + self.assertNotIn("trust the Ix hooks", result.stdout) + self.assertIn("hooks.json is unchanged", result.stdout) + + def test_a_locally_edited_hook_file_is_restored(self) -> None: + self.ok() + (self.codex / "hooks" / "post_tool_use.py").write_text("# edited\n", encoding="utf-8") + self.ok() + self.assert_current_install(synthesized=False) + + +class Upgrade(InstallerHarness): + def test_from_a_pre_24_install(self) -> None: + """2.4.1: copied server.py registered by hand, codex_hooks flag, `-lc`, `Bash`.""" + self.synthesize("2.4.1", legacy_mcp=True) + result = self.ok() + self.assert_current_install() + + self.assertIn("--force", self.mcp_calls()[0], "the stale server.py entry needs --force") + self.assertNotIn("server.py", self.config()) + self.assertIn('command = "ix"', self.config()) + self.assertFalse((self.codex / "mcp" / "server.py").exists()) + self.assertFalse((self.codex / "mcp").exists()) + self.assertIn("codex_hooks = true", self.config(), "config.toml is the user's to edit") + self.assertIn("written by an older installer", result.stdout) + self.assertIn("trust the Ix hooks", result.stdout) + + def test_from_2_4_2(self) -> None: + self.synthesize("2.4.2") + self.ok() + self.assert_current_install() + + def test_from_2_4_3(self) -> None: + self.synthesize("2.4.3") + self.ok() + self.assert_current_install() + self.assertNotIn("--force", self.mcp_calls()[0], "nothing stale to replace") + + def test_from_a_2_4_x_windows_install(self) -> None: + """2.4.x on Windows wrote `"python" "...\\_launch.py" name` as the command.""" + self.synthesize("2.4.2", windows=True) + self.ok() + self.assert_current_install() + for handler in plugin_handlers(self.hooks_json()): + self.assertNotIn("Python311", handler["command"]) + + def test_a_retired_file_in_the_manifest_is_removed_and_a_users_file_is_not(self) -> None: + self.ok() + manifest_path = self.codex / "ix-plugin-version.json" + manifest = json.loads(manifest_path.read_text()) + manifest["files"].append(".codex/hooks/retired_hook.py") + manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + (self.codex / "hooks" / "retired_hook.py").write_text("# old\n", encoding="utf-8") + (self.codex / "hooks" / "my_own.py").write_text("# mine\n", encoding="utf-8") + + self.ok() + self.assertFalse((self.codex / "hooks" / "retired_hook.py").exists()) + self.assertTrue((self.codex / "hooks" / "my_own.py").exists()) + + @unittest.skipIf(os.name == "nt", "symlinks need a privilege on Windows") + def test_from_a_symlink_install_to_copies(self) -> None: + self.ok("--home", "--plugin", "--hooks", "--mode", "symlink") + self.assertTrue((self.codex / "hooks" / "common.py").is_symlink()) + self.ok("--home", "--plugin", "--hooks") + self.assertFalse((self.codex / "hooks" / "common.py").is_symlink()) + self.assertFalse((self.codex / "hooks.json").is_symlink()) + self.assertEqual(SHIPPED_HOOKS_JSON.read_bytes(), (self.codex / "hooks.json").read_bytes()) + + +class UsersOwnHooks(InstallerHarness): + def test_a_users_hooks_json_is_merged_into_not_replaced(self) -> None: + mine = { + "hooks": { + "PreToolUse": [{"matcher": "Bash", "hooks": [USER_HOOK]}], + "SessionEnd": [{"hooks": [{"type": "command", "command": "echo bye"}]}], + } + } + (self.codex / "hooks.json").write_text(json.dumps(mine), encoding="utf-8") + self.ok() + payload = self.hooks_json() + self.assertEqual(mine["hooks"]["SessionEnd"], payload["hooks"]["SessionEnd"]) + self.assertEqual( + {"matcher": "Bash", "hooks": [USER_HOOK]}, payload["hooks"]["PreToolUse"][0], + "the user's hook must keep its position", + ) + self.assertEqual(5, len(plugin_handlers(payload))) + + self.ok() + self.assertEqual(payload, self.hooks_json(), "a second run must not duplicate anything") + + def test_an_old_plugin_entry_beside_the_users_is_replaced_in_place(self) -> None: + payload = json.loads(old_hooks_json()) + payload["hooks"]["PreToolUse"].insert(0, {"matcher": "Bash", "hooks": [USER_HOOK]}) + (self.codex / "hooks.json").write_text(json.dumps(payload), encoding="utf-8") + self.ok() + pre = self.hooks_json()["hooks"]["PreToolUse"] + self.assertEqual([USER_HOOK], pre[0]["hooks"]) + self.assertEqual("Bash|apply_patch", pre[1]["matcher"]) + self.assertEqual(2, len(pre)) + + +class Mcp(InstallerHarness): + def test_ix_missing_is_a_warning_and_the_old_server_is_kept(self) -> None: + """Nothing replaced the registration, so the file it launches must stay.""" + self.synthesize("2.4.1", legacy_mcp=True) + result = self.ok(ix=False) + self.assert_current_install() + self.assertIn("install the Ix CLI", result.stdout) + self.assertTrue((self.codex / "mcp" / "server.py").exists()) + self.assertEqual([], self.mcp_calls()) + + def test_a_failing_ix_mcp_install_is_reported(self) -> None: + result = self.ok(FAKE_IX_MODE="fail") + self.assertIn("failed (exit 1)", result.stdout) + self.assertIn("codex mcp add exploded", result.stdout) + self.assert_current_install(synthesized=False) + + def test_a_hanging_ix_mcp_install_is_stopped(self) -> None: + result = self.ok(FAKE_IX_MODE="hang", IX_CODEX_MCP_TIMEOUT="1") + self.assertIn("did not finish within 1s", result.stdout) + self.assert_current_install(synthesized=False) + + def test_a_conflicting_server_is_left_alone_without_force(self) -> None: + (self.codex / "config.toml").write_text( + '[mcp_servers.ix-memory]\ncommand = "node"\nargs = ["/opt/other/server.js"]\n', + encoding="utf-8", + ) + result = self.ok() + self.assertIn("left alone", result.stdout) + self.assertIn("/opt/other/server.js", self.config()) + self.ok("--home", "--force") + self.assertIn('command = "ix"', self.config()) + + @unittest.skipIf(tomllib is None, "the fallback proves its edit with tomllib") + def test_a_cli_without_force_gets_the_entry_rewritten(self) -> None: + self.synthesize("2.4.1", legacy_mcp=True) + result = self.ok(FAKE_IX_MODE="noforce") + parsed = tomllib.loads(self.config()) + entry = parsed["mcp_servers"]["ix-memory"] + # As `ix mcp install` writes it: the bare name, or on Windows the + # resolved shim, since nothing there resolves `ix` to `ix.cmd` for Codex. + if os.name != "nt": + self.assertEqual("ix", entry["command"]) + self.assertEqual(["mcp"], entry["args"]) + self.assertEqual("o3", parsed["model"], "the rest of config.toml must survive") + self.assertIn("Replaced the old ix-memory server", result.stdout) + + +class NoHalfInstalls(InstallerHarness): + def test_an_unmergeable_hooks_json_stops_the_install_before_any_write(self) -> None: + (self.codex / "hooks.json").write_text("{ not json", encoding="utf-8") + before = snapshot(self.home) + result = self.install() + self.assertEqual(1, result.returncode) + self.assertNotIn("Traceback", result.stderr) + self.assertIn("not valid JSON", result.stderr) + self.assertEqual(before, snapshot(self.home), "a failed install must write nothing") + + def test_a_foreign_marketplace_entry_needs_force(self) -> None: + market = self.home / ".agents" / "plugins" / "marketplace.json" + market.parent.mkdir(parents=True) + market.write_text(json.dumps({"plugins": [ + {"name": "ix-memory", "source": {"source": "local", "path": "./dev/ix-memory"}} + ]}), encoding="utf-8") + before = snapshot(self.home) + result = self.install() + self.assertEqual(1, result.returncode) + self.assertEqual(before, snapshot(self.home)) + self.ok("--home", "--force") + self.assertIn("./.codex/plugins/ix-memory", market.read_text()) + + def test_a_failure_partway_through_the_swap_is_rolled_back(self) -> None: + self.synthesize("2.4.2") + before = snapshot(self.home) + + spec = importlib.util.spec_from_file_location("ix_installer_upgrade", INSTALLER) + installer = importlib.util.module_from_spec(spec) + spec.loader.exec_module(installer) + + calls = {"n": 0} + real_replace = os.replace + + def flaky(src, dst): + calls["n"] += 1 + if calls["n"] == 5: + raise OSError("disk went away") + return real_replace(src, dst) + + env = self.env() + with patch.dict(os.environ, env, clear=True), \ + patch.object(installer, "_replace", flaky), \ + patch("sys.stdout"), patch("sys.stderr"): + code = installer.main(["--home", "--plugin", "--hooks"]) + self.assertEqual(1, code) + self.assertGreater(calls["n"], 5, "the failure was never reached") + self.assertEqual(before, snapshot(self.home), "the target must be as it was") + + +class LegacyCache(InstallerHarness): + def test_the_old_shared_cache_dir_is_removed(self) -> None: + old = self.tmp / "ix-codex-hooks" + old.mkdir() + for name in ("ix-status.json", "ix-pro.json", "ix-briefing.txt", "ix-runtime-health.json"): + (old / name).write_text("{}", encoding="utf-8") + result = self.ok() + self.assertFalse(old.exists()) + self.assertIn("Removed the old hook cache", result.stdout) + + def test_a_dir_with_anything_else_in_it_is_left_alone(self) -> None: + old = self.tmp / "ix-codex-hooks" + old.mkdir() + (old / "ix-status.json").write_text("{}", encoding="utf-8") + (old / "not-ours.txt").write_text("x", encoding="utf-8") + self.ok() + self.assertTrue((old / "not-ours.txt").exists()) + self.assertTrue((old / "ix-status.json").exists()) + + @unittest.skipIf(os.name == "nt", "symlinks need a privilege on Windows") + def test_a_symlink_is_never_followed(self) -> None: + elsewhere = self.root / "elsewhere" + elsewhere.mkdir() + (elsewhere / "ix-status.json").write_text("{}", encoding="utf-8") + (self.tmp / "ix-codex-hooks").symlink_to(elsewhere) + self.ok() + self.assertTrue((elsewhere / "ix-status.json").exists()) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/tests/test_ix_argv_resolution.py b/tests/test_ix_argv_resolution.py index 46e9676..c7aa3e4 100644 --- a/tests/test_ix_argv_resolution.py +++ b/tests/test_ix_argv_resolution.py @@ -281,9 +281,12 @@ def fake_run(argv, *args, **kwargs): with tempfile.TemporaryDirectory() as target: argv = ["install_codex_integration.py", "--repo", target, "--mcp"] + # CODEX_HOME too: the installer reads the Codex config to recognise a + # stale registration, and that must be a fixture, never the real one. with patch.object(self.installer.shutil, "which", return_value=which_returns), \ patch.object(self.installer.subprocess, "run", fake_run), \ patch.object(sys, "argv", argv), \ + patch.dict(os.environ, {"CODEX_HOME": str(Path(target) / "codex-home")}), \ redirect_stdout(io.StringIO()) as out: self.installer.main() return calls, out.getvalue() @@ -294,7 +297,7 @@ def test_registers_with_the_resolved_path_not_the_bare_name(self) -> None: self.assertEqual(1, len(register), f"expected one registration, got {calls}") self.assertEqual( [WINDOWS_SHIM, "mcp", "install", "--host", "codex"], - register[0], + register[0][:5], "the bare name never resolves through CreateProcess on Windows", ) diff --git a/tests/test_windows_hook_launch.py b/tests/test_windows_hook_launch.py index 63bc4e9..2e2bb0b 100644 --- a/tests/test_windows_hook_launch.py +++ b/tests/test_windows_hook_launch.py @@ -9,8 +9,15 @@ `common.py` by #19) unreachable, because the code carrying that fix never ran. The fix has two halves of its own: `.codex/hooks/_launch.py` does the walk-up -the shell one-liner used to do, and the installer rewrites `hooks.json` on -Windows to invoke it with an absolute interpreter and an absolute path. +the shell one-liner used to do, and the installer gives every hook in +`hooks.json` a `commandWindows` that invokes it with an absolute interpreter and +an absolute path. + +That command runs under PowerShell: Codex 0.155 runs hooks through the session +shell, which on Windows is `powershell.exe -NoProfile -Command` +(codex-rs/core/src/session/mod.rs, shell-command/src/shell_detect.rs at +rust-v0.155.1). The 2.4.x form, `"python" "launcher" name`, is a string followed +by stray tokens there -- a parse error -- so it needs the call operator `&`. """ from __future__ import annotations @@ -48,9 +55,9 @@ def load_module(path: Path, name: str): SHIPPED_HOOKS_JSON = REPO_ROOT / ".codex" / "hooks.json" -def commands_in(payload: dict) -> list[str]: +def commands_in(payload: dict, key: str = "command") -> list[str]: return [ - hook["command"] + hook[key] for blocks in payload["hooks"].values() for block in blocks for hook in block["hooks"] @@ -112,23 +119,39 @@ def render(self): return installer.render_hooks_json(self.target, self.launch) def test_is_a_no_op_off_windows(self) -> None: - # None means "install the source unchanged", which keeps install_file's - # content comparison — and re-running without --force — working. + # None means "install the source unchanged". os.name = "posix" before = self.target.read_text(encoding="utf-8") self.assertIsNone(self.render()) self.assertEqual(before, self.target.read_text(encoding="utf-8")) - def test_removes_every_shell_invocation(self) -> None: + def test_every_hook_gets_a_shell_free_windows_command(self) -> None: os.name = "nt" rendered = self.render() self.assertIsNotNone(rendered) - commands = commands_in(json.loads(rendered)) + commands = commands_in(json.loads(rendered), "commandWindows") self.assertTrue(commands) for command in commands: self.assertNotIn("/bin/sh", command) self.assertIn("_launch.py", command) - self.assertTrue(command.startswith(f'"{sys.executable}"'), command) + self.assertTrue(command.startswith(f"& '{sys.executable}' "), command) + + def test_the_windows_command_is_a_powershell_invocation(self) -> None: + """Codex runs it as `powershell.exe -NoProfile -Command `. + + A leading quoted path without `&` is a string expression to PowerShell, + so the 2.4.x command never launched anything there. + """ + os.name = "nt" + for command in commands_in(json.loads(self.render()), "commandWindows"): + self.assertRegex(command, r"^& '[^']+' '[^']+' [a-z_]+$") + + def test_keeps_the_posix_command_for_other_machines(self) -> None: + # A repo-local hooks.json is shared through git; Codex picks + # commandWindows only on Windows. + os.name = "nt" + source = commands_in(json.loads(SHIPPED_HOOKS_JSON.read_text(encoding="utf-8"))) + self.assertEqual(source, commands_in(json.loads(self.render()))) def test_preserves_which_hook_each_event_fires(self) -> None: # A rewrite that pointed every event at the wrong handler would be worse @@ -147,7 +170,7 @@ def test_preserves_which_hook_each_event_fires(self) -> None: payload = json.loads(self.render()) actual = { event: [ - hook["command"].rsplit(" ", 1)[1] + hook["commandWindows"].rsplit(" ", 1)[1] for block in blocks for hook in block["hooks"] ] @@ -161,9 +184,7 @@ def test_is_idempotent(self) -> None: self.assertIsNotNone(once) # Feed the rendered output back in: a second pass must find nothing to do. self.target.write_text(once, encoding="utf-8") - self.assertIsNone( - self.render(), "the `/bin/sh` guard is what stops a self-referential command" - ) + self.assertIsNone(self.render(), "a rendered file must render to itself") def test_quotes_paths_containing_spaces(self) -> None: """#349 is a live report from a profile at `C:\\Users\\Win 10`.""" @@ -175,9 +196,15 @@ def test_quotes_paths_containing_spaces(self) -> None: # stopped raising in 3.12 -- green locally, red on a 3.11 runner. launch = PureWindowsPath(r"C:\Users\Win 10\.codex\hooks\_launch.py") payload = json.loads(installer.render_hooks_json(self.target, launch)) - command = payload["hooks"]["SessionStart"][0]["hooks"][0]["command"] - self.assertTrue(command.startswith(r'"C:\Users\Win 10\Python\python.exe" ')) - self.assertIn(f'"{launch}"', command) + command = payload["hooks"]["SessionStart"][0]["hooks"][0]["commandWindows"] + self.assertTrue(command.startswith(r"& 'C:\Users\Win 10\Python\python.exe' ")) + self.assertIn(f"'{launch}'", command) + + def test_a_quote_or_dollar_in_a_path_stays_literal(self) -> None: + """Single-quoted PowerShell strings expand nothing; a `'` is doubled.""" + self.assertEqual("'C:\\a$b'", installer.powershell_literal("C:\\a$b")) + self.assertEqual("'C:\\O''Brien'", installer.powershell_literal("C:\\O'Brien")) + self.assertEqual("'x\u2019\u2019y'", installer.powershell_literal("x\u2019y")) def test_leaves_an_unparseable_file_alone(self) -> None: os.name = "nt" @@ -212,7 +239,7 @@ def test_a_byte_order_mark_does_not_skip_the_rewrite(self) -> None: ) rendered = self.render() self.assertIsNotNone(rendered) - for command in commands_in(json.loads(rendered)): + for command in commands_in(json.loads(rendered), "commandWindows"): self.assertNotIn("/bin/sh", command) @@ -237,18 +264,12 @@ def install(self): capture_output=True, text=True, ) - def installed_commands(self) -> list[str]: + def installed_commands(self, key: str = "command") -> list[str]: text = (self.target / ".codex" / "hooks.json").read_text(encoding="utf-8-sig") - return commands_in(json.loads(text)) + return commands_in(json.loads(text), key) def test_a_second_install_is_not_an_error(self) -> None: - """The documented Windows one-liner never passes --force. - - Rewriting after install_file left the installed file permanently - different from the source, so the next run's content comparison raised - FileExistsError — after install_plugin had written and before install_mcp - ran, i.e. a partial install with a traceback. - """ + """The documented Windows one-liner never passes --force.""" first = self.install() self.assertEqual(0, first.returncode, first.stderr) before = (self.target / ".codex" / "hooks.json").read_bytes() @@ -294,9 +315,9 @@ def test_the_launcher_is_installed_beside_the_hooks(self) -> None: self.assertTrue((self.target / ".codex" / "hooks" / "_launch.py").is_file()) @unittest.skipUnless(os.name == "nt", "the rewrite is Windows-only") - def test_no_hook_still_needs_bin_sh(self) -> None: + def test_every_hook_has_a_shell_free_windows_command(self) -> None: self.assertEqual(0, self.install().returncode) - commands = self.installed_commands() + commands = self.installed_commands("commandWindows") self.assertTrue(commands) for command in commands: self.assertNotIn("/bin/sh", command) @@ -313,30 +334,6 @@ def test_off_windows_the_installed_file_matches_the_source(self) -> None: ) -class InstallRendered(unittest.TestCase): - """The write path hooks.json takes on Windows, which bypasses install_file.""" - - def setUp(self) -> None: - self._dir = tempfile.TemporaryDirectory() - self.addCleanup(self._dir.cleanup) - self.target = Path(self._dir.name) / "nested" / "hooks.json" - - def test_matching_content_is_a_no_op(self) -> None: - installer.install_rendered(self.target, "same", force=False) - installer.install_rendered(self.target, "same", force=False) - self.assertEqual("same", self.target.read_text(encoding="utf-8")) - - def test_differing_content_needs_force(self) -> None: - installer.install_rendered(self.target, "first", force=False) - with self.assertRaises(FileExistsError): - installer.install_rendered(self.target, "second", force=False) - self.assertEqual( - "first", self.target.read_text(encoding="utf-8"), "content was clobbered" - ) - installer.install_rendered(self.target, "second", force=True) - self.assertEqual("second", self.target.read_text(encoding="utf-8")) - - class Launcher(unittest.TestCase): def setUp(self) -> None: self._dir = tempfile.TemporaryDirectory() From 3013db37f937bfef38a9df66a0db9239bd16441e Mon Sep 17 00:00:00 2001 From: KageBinary Date: Thu, 1 Oct 2026 18:39:50 -0700 Subject: [PATCH 2/2] chore: bump plugin version to 2.4.4 Co-Authored-By: Claude Opus 5.5 (1M context) --- .agents/plugins/marketplace.json | 2 +- plugins/ix-memory/.codex-plugin/plugin.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.agents/plugins/marketplace.json b/.agents/plugins/marketplace.json index c9d4b79..fba301e 100644 --- a/.agents/plugins/marketplace.json +++ b/.agents/plugins/marketplace.json @@ -11,7 +11,7 @@ "path": "./plugins/ix-memory" }, "description": "Ix Memory integration — cognitive skills, agent playbooks, and Codex hooks for graph-aware engineering", - "version": "2.4.3", + "version": "2.4.4", "policy": { "installation": "AVAILABLE", "authentication": "ON_INSTALL" diff --git a/plugins/ix-memory/.codex-plugin/plugin.json b/plugins/ix-memory/.codex-plugin/plugin.json index 8e0e584..aafa346 100644 --- a/plugins/ix-memory/.codex-plugin/plugin.json +++ b/plugins/ix-memory/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "ix-memory", - "version": "2.4.3", + "version": "2.4.4", "description": "Ix Memory integration — transforms Codex into a graph-reasoning agent with cognitive skills for understanding, investigation, impact analysis, planning, debugging, architecture auditing, and deep documentation synthesis.", "author": { "name": "Ix Memory"