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/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/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" 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()