From 831d20f64f4331247fcbfeaeeb1a214fd1115d53 Mon Sep 17 00:00:00 2001 From: kuse <3133746534@qq.com> Date: Fri, 16 Oct 2026 18:02:05 +0800 Subject: [PATCH 1/2] fix(setup-preflight): read the version floor through each harness's own listing read_installed only ever ran 'claude plugin list --json', so on Codex, Gemini CLI or Copilot CLI the floor was never checked: the missing binary collapsed the listing to unknown and _floor_findings said nothing. Probe each harness's own listing command in order and let the first binary that exists answer for the machine; a listing it cannot read stays unknown. Refs #1641. --- .../skills/setup/setup_preflight/cli.py | 81 +++++++++++------ tools/setup-preflight/tests/test_cli.py | 86 +++++++++++++++++++ 2 files changed, 140 insertions(+), 27 deletions(-) diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/cli.py b/plugins/magpie-setup/skills/setup/setup_preflight/cli.py index b1123c341..c293426f6 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/cli.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/cli.py @@ -47,50 +47,77 @@ ) from .lockfile import MalformedLock +#: Each harness's own plugin-listing command, in probe order. The floor +#: must be readable on every harness, not only Claude Code: a missing +#: `claude` binary on Codex or Gemini CLI is that harness answering with +#: its own listing (`install.md` Step M5), never *unknown*. The first +#: binary that exists decides the answer; a listing it cannot read is +#: unknown, and a machine running none of them stays unknown too. +HARNESS_LISTINGS: tuple[tuple[str, list[str]], ...] = ( + ("claude-code", ["claude", "plugin", "list", "--json"]), + ("codex", ["codex", "plugin", "list"]), + ("gemini", ["gemini", "extensions", "list"]), +) + + +def _parse_listing(text: str) -> dict[str, str] | None: + """One harness listing, parsed into the shared `{name: version}` map. + + Shared by every harness's listing command: `None` for unknown (the + output did not parse, or an empty parsed listing leaves the sandbox + read-denial ambiguity standing), never an empty map. + """ + try: + listing = json.loads(text) + except json.JSONDecodeError: + return None + if not isinstance(listing, list): + return None + out: dict[str, str] = {} + for item in listing: + if not isinstance(item, dict): + return None + version = item.get("version") + if isinstance(item.get("name"), str) and isinstance(version, str): + out[item["name"]] = version + # A listing that parses but is empty leaves the sandbox read-denial + # ambiguity standing, so we report unknown rather than an empty map. + return out or None + def read_installed(explicit: str | None) -> dict[str, str] | None: """The installed plugin map, or `None` for *unknown* — never `{}` for it. `--plugin-list` lets the caller supply what it already read; otherwise - the harness CLI is asked directly. Every failure mode collapses to - `None`: the command missing, a non-zero exit, unparsable output, or a - sandbox that denies the plugin cache. The one case that is *not* - unknown is a listing that parsed and was genuinely empty, which only a - harness with no plugins installed produces. + each harness's own listing command is probed in order and the first + binary that exists answers for the machine. Every failure mode + collapses to `None`: the command missing, a non-zero exit, unparsable + output, or a sandbox that denies the plugin cache. The one case that + is *not* unknown is a listing that parsed and was genuinely empty, + which only a harness with no plugins installed produces. """ if explicit is not None: text = sys.stdin.read() if explicit == "-" else Path(explicit).read_text(encoding="utf-8") - else: + return _parse_listing(text) + for _harness, argv in HARNESS_LISTINGS: try: completed = subprocess.run( - ["claude", "plugin", "list", "--json"], + argv, + check=False, capture_output=True, text=True, timeout=20, - check=False, ) except (OSError, subprocess.SubprocessError): - return None + # The binary is not on PATH: this is not the running harness, + # so the next harness's own command decides. + continue if completed.returncode != 0: return None - text = completed.stdout - try: - parsed = json.loads(text) - except json.JSONDecodeError: - return None - if not isinstance(parsed, list): - return None - installed: dict[str, str] = {} - for entry in parsed: - if not isinstance(entry, dict): - return None - name, version = entry.get("name"), entry.get("version") - if isinstance(name, str) and isinstance(version, str): - installed[name] = version - # An empty *parsed* list inside a sandbox is the read-denied case, and - # is indistinguishable from a genuinely plugin-free harness. Unknown is - # the safe reading of the ambiguity: it proposes nothing. - return installed or None + return _parse_listing(completed.stdout) + # No harness CLI exists on this machine: nothing was ever installed + # with one, and unknown stays the safe reading. + return None def build_parser() -> argparse.ArgumentParser: diff --git a/tools/setup-preflight/tests/test_cli.py b/tools/setup-preflight/tests/test_cli.py index 76c788cb1..7cdf1d281 100644 --- a/tools/setup-preflight/tests/test_cli.py +++ b/tools/setup-preflight/tests/test_cli.py @@ -21,6 +21,7 @@ from __future__ import annotations import json +import subprocess from pathlib import Path import pytest @@ -138,3 +139,88 @@ def explode(*_args: object, **_kwargs: object) -> None: monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) assert read_installed(None) is None + + +# --- read_installed: every harness answers with its own listing ---------------------- + + +def test_a_missing_claude_binary_falls_through_to_codex( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A missing `claude` binary on another harness is not unknown: that + harness answers with its own listing (issue #1641).""" + seen: list[list[str]] = [] + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + seen.append(list(argv)) + if argv[0] == "claude": + raise FileNotFoundError("claude") + completed = subprocess.CompletedProcess[str](argv, 0) + completed.stdout = '[{"name": "magpie-setup", "version": "0.3.1"}]' + return completed + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None) == {"magpie-setup": "0.3.1"} + assert seen == [["claude", "plugin", "list", "--json"], ["codex", "plugin", "list"]] + + +def test_no_harness_cli_at_all_is_unknown(monkeypatch: pytest.MonkeyPatch) -> None: + """A machine where none of the listing commands exist stays unknown.""" + + def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: + raise FileNotFoundError("no harness here") + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) + assert read_installed(None) is None + + +def test_a_found_harness_that_fails_is_unknown_not_a_fallthrough( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The first binary that exists answers for the machine: a listing it + cannot produce stays unknown rather than borrowing another harness's.""" + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + if argv[0] == "claude": + raise FileNotFoundError("claude") + completed = subprocess.CompletedProcess[str](argv, 1) + completed.stdout = "" + return completed + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None) is None + + +def test_an_unparsable_listing_from_the_found_harness_is_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A found harness whose listing is not the shared JSON shape reads as + unknown — the checker proposes nothing it cannot read.""" + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + if argv[0] == "claude": + raise FileNotFoundError("claude") + completed = subprocess.CompletedProcess[str](argv, 0) + completed.stdout = "magpie-setup 0.3.1 (marketplace)" + return completed + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None) is None + + +def test_a_present_claude_listing_is_not_second_guessed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A readable Claude Code listing ends the probe: later harnesses are + never asked, so the established behaviour is unchanged.""" + seen: list[list[str]] = [] + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + seen.append(list(argv)) + completed = subprocess.CompletedProcess[str](argv, 0) + completed.stdout = '[{"name": "magpie-setup", "version": "0.2.0"}]' + return completed + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None) == {"magpie-setup": "0.2.0"} + assert seen == [["claude", "plugin", "list", "--json"]] From 9a14d01a6ef1021039f08e7bec9dc3cb4d164d91 Mon Sep 17 00:00:00 2001 From: kuse <3133746534@qq.com> Date: Sat, 17 Oct 2026 14:42:10 +0800 Subject: [PATCH 2/2] fix(setup-preflight): detect the running harness and read its own listing Review feedback on #1673: a fixed PATH probe could answer from the wrong harness, and the Codex and Gemini listings never parsed without their JSON flags, so the floor stayed unchecked where the issue needs it. Detect the running harness from its own environment markers (the canonical innermost-first order from tools/adversarial-review), run only that harness's listing (`codex plugin list --json`, `gemini extensions list -o json`), and return the harness id with the map so the below-floor finding names that harness's install and update commands (step-3.md now carries the per-harness table). A harness with no listing CLI (Copilot) and a listing that cannot be produced - missing, timed out, refused, non-zero, unparsable - all stay unknown; nothing is probed in a fixed order. Tests cover each marker, the innermost-first order, the JSON flags, every unknown path, and the harness named in the finding and the cache key; the preflight-floor eval cases gain the below-floor Codex and Gemini installs issue #1641 asked for (case-5 updated, case-9 added). --- .../skills/setup/setup_preflight/cli.py | 123 ++++++---- .../skills/setup/setup_preflight/core.py | 44 ++-- .../setup/setup_preflight/sections/step-3.md | 24 +- tools/setup-preflight/README.md | 17 +- tools/setup-preflight/tests/test_cli.py | 222 ++++++++++++++---- tools/setup-preflight/tests/test_core.py | 24 ++ .../case-5-no-claude-cli/expected.json | 2 +- .../fixtures/case-5-no-claude-cli/report.md | 7 +- .../case-9-below-floor-codex/case-meta.json | 1 + .../case-9-below-floor-codex/expected.json | 1 + .../case-9-below-floor-codex/report.md | 13 + 11 files changed, 351 insertions(+), 127 deletions(-) create mode 100644 tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/case-meta.json create mode 100644 tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/expected.json create mode 100644 tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/report.md diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/cli.py b/plugins/magpie-setup/skills/setup/setup_preflight/cli.py index c293426f6..31ade8803 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/cli.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/cli.py @@ -33,8 +33,10 @@ import argparse import json +import os import subprocess import sys +from collections.abc import Mapping from pathlib import Path from .core import ( @@ -47,19 +49,37 @@ ) from .lockfile import MalformedLock -#: Each harness's own plugin-listing command, in probe order. The floor -#: must be readable on every harness, not only Claude Code: a missing -#: `claude` binary on Codex or Gemini CLI is that harness answering with -#: its own listing (`install.md` Step M5), never *unknown*. The first -#: binary that exists decides the answer; a listing it cannot read is -#: unknown, and a machine running none of them stays unknown too. -HARNESS_LISTINGS: tuple[tuple[str, list[str]], ...] = ( - ("claude-code", ["claude", "plugin", "list", "--json"]), - ("codex", ["codex", "plugin", "list"]), - ("gemini", ["gemini", "extensions", "list"]), +#: How to notice which harness is running, and that harness's own +#: plugin-listing command (`install.md` Step M2/M5). The markers mirror +#: the canonical self-detection order in `tools/adversarial-review`: a +#: harness started from inside another inherits the outer one's variables, +#: so the innermost candidates are checked first and Claude Code's widely +#: inherited `CLAUDECODE` comes last. Only the running harness's listing +#: runs — a machine may have several installed, and whichever comes first +#: in a fixed probe order is not the one whose versions this session runs. +#: Copilot installs from a repo URL or catalogue and has no listing CLI, +#: so its entry has none: the floor stays unknown there rather than +#: guessing what a listing would print. +HARNESS_LISTINGS: tuple[ + tuple[str, tuple[tuple[str, str | None], ...], list[str] | None], ... +] = ( + ("codex", (("CODEX_SANDBOX", None), ("CODEX_THREAD_ID", None)), ["codex", "plugin", "list", "--json"]), + ("copilot", (("COPILOT_CLI", None),), None), + ("gemini", (("GEMINI_CLI", "1"),), ["gemini", "extensions", "list", "-o", "json"]), + ("claude", (("CLAUDECODE", "1"),), ["claude", "plugin", "list", "--json"]), ) +def _running_harness(env: Mapping[str, str]) -> tuple[str | None, list[str] | None]: + """The harness these environment variables are running, and its listing.""" + for harness, markers, argv in HARNESS_LISTINGS: + for var, want in markers: + value = env.get(var) + if value and (want is None or value == want): + return harness, argv + return None, None + + def _parse_listing(text: str) -> dict[str, str] | None: """One harness listing, parsed into the shared `{name: version}` map. @@ -85,39 +105,54 @@ def _parse_listing(text: str) -> dict[str, str] | None: return out or None -def read_installed(explicit: str | None) -> dict[str, str] | None: - """The installed plugin map, or `None` for *unknown* — never `{}` for it. - - `--plugin-list` lets the caller supply what it already read; otherwise - each harness's own listing command is probed in order and the first - binary that exists answers for the machine. Every failure mode - collapses to `None`: the command missing, a non-zero exit, unparsable - output, or a sandbox that denies the plugin cache. The one case that - is *not* unknown is a listing that parsed and was genuinely empty, - which only a harness with no plugins installed produces. +def read_installed( + explicit: str | None, env: Mapping[str, str] | None = None +) -> tuple[str | None, dict[str, str] | None]: + """The running harness and its installed plugin map, or `None` for + *unknown* — never `{}` for it. The pair is a coherent answer: both + are `None` unless the running harness's own listing was read and + parsed. + + `--plugin-list` lets the caller supply what it already read; the + harness is then not known, and the map stands alone. Otherwise the + running harness is detected from its own environment markers and only + its listing command runs — no other harness's listing may answer for + it. Every failure mode collapses to `(None, None)`: the harness + undetectable, a listing CLI the running harness does not have + (Copilot), the command missing, a non-zero exit, a timeout or a + refusal from the sandbox, unparsable output, or a sandbox that denies + the plugin cache. The one case that is *not* unknown is a listing + that parsed and was genuinely empty, which only a harness with no + plugins installed produces. """ if explicit is not None: text = sys.stdin.read() if explicit == "-" else Path(explicit).read_text(encoding="utf-8") - return _parse_listing(text) - for _harness, argv in HARNESS_LISTINGS: - try: - completed = subprocess.run( - argv, - check=False, - capture_output=True, - text=True, - timeout=20, - ) - except (OSError, subprocess.SubprocessError): - # The binary is not on PATH: this is not the running harness, - # so the next harness's own command decides. - continue - if completed.returncode != 0: - return None - return _parse_listing(completed.stdout) - # No harness CLI exists on this machine: nothing was ever installed - # with one, and unknown stays the safe reading. - return None + return None, _parse_listing(text) + harness, argv = _running_harness(os.environ if env is None else env) + if argv is None: + # No harness is running here, or the one running has no listing + # CLI (Copilot installs from a repo URL): unknown, and no other + # harness's listing may be probed instead. + return None, None + try: + completed = subprocess.run( + argv, + check=False, + capture_output=True, + text=True, + timeout=20, + ) + except (OSError, subprocess.SubprocessError): + # The listing cannot be produced: the binary is missing from the + # harness it belongs to, it times out, or the sandbox refuses to + # execute it. Unknown — never another harness's answer. + return None, None + if completed.returncode != 0: + return None, None + parsed = _parse_listing(completed.stdout) + if parsed is None: + return None, None + return harness, parsed def build_parser() -> argparse.ArgumentParser: @@ -138,7 +173,7 @@ def build_parser() -> argparse.ArgumentParser: parser.add_argument( "--plugin-list", metavar="PATH", - help="`claude plugin list --json` output to use instead of running it ('-' for stdin)", + help="harness plugin listing JSON to use instead of running it ('-' for stdin)", ) parser.add_argument( "--verify-interval-days", @@ -171,13 +206,13 @@ def main(argv: list[str] | None = None) -> int: root: Path = args.project_root try: - installed = read_installed(args.plugin_list) + harness, installed = read_installed(args.plugin_list) if args.no_cache: from .core import project_findings - project, cached = project_findings(root, installed), False + project, cached = project_findings(root, installed, harness), False else: - project, cached = cached_project_findings(root, installed) + project, cached = cached_project_findings(root, installed, harness=harness) findings = [ *project, *skill_findings(root, args.skill, args.surface_hash, args.requires), diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/core.py b/plugins/magpie-setup/skills/setup/setup_preflight/core.py index 412e0aaad..407dbaca3 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/core.py +++ b/plugins/magpie-setup/skills/setup/setup_preflight/core.py @@ -152,7 +152,9 @@ def _snapshot_findings(lock: Lock, root: Path) -> list[Finding]: return [] -def _floor_findings(lock: Lock, installed: dict[str, str] | None) -> list[Finding]: +def _floor_findings( + lock: Lock, installed: dict[str, str] | None, harness: str | None +) -> list[Finding]: if lock.url and lock.url != TRUSTED_MARKETPLACE: return [Finding("project", "untrusted-marketplace", "step-3", {"url": lock.url})] if installed is None: @@ -177,7 +179,7 @@ def _floor_findings(lock: Lock, installed: dict[str, str] | None) -> list[Findin "project", "below-floor", "step-3", - {"missing": missing, "outdated": outdated, "restart_required": True}, + {"missing": missing, "outdated": outdated, "restart_required": True, "harness": harness}, ) ] @@ -223,8 +225,16 @@ def legacy_layout_findings(root: Path) -> list[Finding]: ] -def project_findings(root: Path, installed: dict[str, str] | None) -> list[Finding]: - """Everything true of the checkout rather than of one skill.""" +def project_findings( + root: Path, installed: dict[str, str] | None, harness: str | None = None +) -> list[Finding]: + """Everything true of the checkout rather than of one skill. + + `harness` is the agent the session is running under, as the floor + check measured it — `None` when the listing came from `--plugin-list` + or could not be read. It travels with the findings so the rules can + name that harness's own install and update commands. + """ lock = load(root / LOCK_NAME) if lock is None: # A supported end state, not a fault: the marketplace install @@ -235,7 +245,7 @@ def project_findings(root: Path, installed: dict[str, str] | None) -> list[Findi if lock.method in SNAPSHOT_METHODS: return _snapshot_findings(lock, root) if lock.method == "marketplace": - return _floor_findings(lock, installed) + return _floor_findings(lock, installed, harness) if lock.method == LOCAL_METHOD: return [] return [Finding("project", "unknown-method", "step-2", {"method": lock.method})] @@ -445,7 +455,7 @@ def isolated_setup_findings( # --- memoising the project scope ---------------------------------------------------- -def _cache_key(root: Path, installed: dict[str, str] | None) -> str: +def _cache_key(root: Path, installed: dict[str, str] | None, harness: str | None) -> str: # Linked worktrees of an adopted repository can share their main # checkout's personal layer, and so this cache file: key it on the # checkout as well, so one worktree never serves another's verdict. @@ -458,6 +468,9 @@ def _cache_key(root: Path, installed: dict[str, str] | None) -> str: # The legacy-layout finding depends on the in-tree directory existing. parts.append("legacy:" + ("1" if legacy_local_dir(root) is not None else "-")) parts.append("installed:" + ("?" if installed is None else json.dumps(installed, sort_keys=True))) + # The below-floor finding names the harness the listing came from, so a + # verdict computed under one harness must not be served to another. + parts.append("harness:" + (harness or "-")) # The verdict is also a function of this code: an upgrade that refreshes # the checker must not keep serving the previous checker's answer. here = Path(__file__).parent @@ -474,6 +487,7 @@ def _cache_key(root: Path, installed: dict[str, str] | None) -> str: def cached_project_findings( root: Path, installed: dict[str, str] | None, + harness: str | None = None, *, now: float | None = None, ) -> tuple[list[Finding], bool]: @@ -481,19 +495,19 @@ def cached_project_findings( Returns `(findings, was_cached)`. The cache lives in the personal layer (`layers.personal_dir`, which in a linked worktree of an adopted - repository may be its main checkout's), is keyed on the checkout, the lock files' identity and - the plugin listing, and expires so that a plugin installed mid-session - is picked up by the next skill rather than at the end of the day. A - project whose personal layer does not exist is not cached at all — - creating it would create the very state whose absence the skill scope - reads as "never configured". + repository may be its main checkout's), is keyed on the checkout, the lock files' identity, + the plugin listing and the harness the listing came from, and expires + so that a plugin installed mid-session is picked up by the next skill + rather than at the end of the day. A project whose personal layer + does not exist is not cached at all — creating it would create the + very state whose absence the skill scope reads as "never configured". """ local_dir = personal_dir(root) if local_dir is None or not local_dir.is_dir(): - return project_findings(root, installed), False + return project_findings(root, installed, harness), False stamp_now = now if now is not None else time.time() - key = _cache_key(root, installed) + key = _cache_key(root, installed, harness) cache_path = local_dir / CACHE_NAME if cache_path.exists(): try: @@ -510,7 +524,7 @@ def cached_project_findings( if isinstance(entries, list): return [Finding(**entry) for entry in entries], True - findings = project_findings(root, installed) + findings = project_findings(root, installed, harness) payload = {"key": key, "at": stamp_now, "findings": [asdict(f) for f in findings]} tmp = cache_path.with_suffix(".tmp") tmp.write_text(json.dumps(payload), encoding="utf-8") diff --git a/plugins/magpie-setup/skills/setup/setup_preflight/sections/step-3.md b/plugins/magpie-setup/skills/setup/setup_preflight/sections/step-3.md index a673d10b8..5d46fc83e 100644 --- a/plugins/magpie-setup/skills/setup/setup_preflight/sections/step-3.md +++ b/plugins/magpie-setup/skills/setup/setup_preflight/sections/step-3.md @@ -16,16 +16,28 @@ the release segment. The reconciliation check in step 4 is gated on the fingerprint, never on this version delta. **Why an unreadable result is *unknown* rather than *absent*.** Inside a -sandboxed session the plugin cache is read-denied and `claude plugin list ---json` returns `[]` there — that reads exactly like "nothing installed" +sandboxed session the plugin cache is read-denied and the running harness's +own listing returns `[]` there — that reads exactly like "nothing installed" but is not. Acting on it would propose installing a project's entire floor on every sandboxed run. -**The actions.** +**The actions.** The `below-floor` finding names the harness its listing +came from (`facts.harness`); the commands differ per client, and naming one +the operator cannot run in this session wastes their turn: -- a floor plugin absent → `claude plugin install @apache-magpie`; -- a floor plugin below `min_version` → `claude plugin update - @apache-magpie`. +| Harness | a floor plugin absent | a floor plugin below `min_version` | +|---|---|---| +| Claude Code | `claude plugin install @apache-magpie` | `claude plugin update @apache-magpie` | +| Codex | `codex plugin add @apache-magpie` | `codex plugin update ` | +| Gemini CLI | `gemini extensions install https://github.com/apache/magpie` | `gemini extensions update magpie` | + +Gemini installs the framework as one extension, so on it the floor is +carried by the `magpie` extension as a whole: update that one extension +rather than naming individual plugins. + +Copilot CLI installs from a repo URL or catalogue and has no plugin CLI to +read a listing or run an action with. There the floor stays unknown: run +nothing, and leave it to the user. Where there is no such CLI, run nothing and print the commands instead. diff --git a/tools/setup-preflight/README.md b/tools/setup-preflight/README.md index df0daeba3..5fdc23775 100644 --- a/tools/setup-preflight/README.md +++ b/tools/setup-preflight/README.md @@ -44,9 +44,10 @@ how to word it, and the prohibitions. Those live in each skill's - **Runtime** — Python 3.11+. Standard library only, deliberately: the module is copied into the user's personal configuration layer and run with bare `python3`, where nothing else is available. -- **CLIs** — none required. The harness CLI (`claude plugin list --json`) - is consulted when present and its absence is a supported state, not an - error. +- **CLIs** — none required. The running harness's own listing command + (detected from its environment markers, e.g. `claude plugin list + --json` under Claude Code) is consulted when present; its absence or + the absence of any harness marker is a supported state, not an error. - **Credentials / auth** — none. - **Network** — none. Every input is a file in the project. - **Optional** — `--plugin-list` accepts a listing the caller already @@ -147,11 +148,11 @@ is settled when the run finishes rather than before it starts. **Unknown is never absent.** A plugin listing that could not be read is `None`, not `{}`. Inside a sandboxed session the plugin cache is -read-denied and `claude plugin list --json` prints `[]`, which reads -exactly like "nothing installed"; acting on it would propose installing a -project's entire floor on every sandboxed run. An empty *parsed* listing -is treated as unknown for the same reason — it is indistinguishable from -the denied case. +read-denied and the running harness's own listing prints `[]`, which +reads exactly like "nothing installed"; acting on it would propose +installing a project's entire floor on every sandboxed run. An empty +*parsed* listing is treated as unknown for the same reason — it is +indistinguishable from the denied case. **A dev build is a version like any other.** Nothing strips or rounds a `.devN` segment. `0.2.0.dev202609110041` is below `0.2.0`, and `0.10.0` diff --git a/tools/setup-preflight/tests/test_cli.py b/tools/setup-preflight/tests/test_cli.py index 7cdf1d281..2e8e92816 100644 --- a/tools/setup-preflight/tests/test_cli.py +++ b/tools/setup-preflight/tests/test_cli.py @@ -109,7 +109,7 @@ def test_every_section_named_is_one_a_reader_can_find(project: Path, capsys) -> def test_unparsable_output_is_unknown(tmp_path: Path) -> None: bad = tmp_path / "bad.json" bad.write_text("not json at all") - assert read_installed(str(bad)) is None + assert read_installed(str(bad)) == (None, None) def test_an_empty_listing_is_unknown_not_an_empty_install(tmp_path: Path) -> None: @@ -118,109 +118,229 @@ def test_an_empty_listing_is_unknown_not_an_empty_install(tmp_path: Path) -> Non harness. Unknown is the safe reading: it proposes nothing.""" empty = tmp_path / "empty.json" empty.write_text("[]") - assert read_installed(str(empty)) is None + assert read_installed(str(empty)) == (None, None) def test_a_real_listing_parses_to_a_name_version_map(tmp_path: Path) -> None: listing = tmp_path / "list.json" listing.write_text('[{"name": "magpie-setup", "version": "0.2.0"}]') - assert read_installed(str(listing)) == {"magpie-setup": "0.2.0"} + assert read_installed(str(listing)) == (None, {"magpie-setup": "0.2.0"}) def test_a_listing_that_is_not_a_list_is_unknown(tmp_path: Path) -> None: listing = tmp_path / "list.json" listing.write_text('{"plugins": []}') - assert read_installed(str(listing)) is None + assert read_installed(str(listing)) == (None, None) -def test_a_missing_harness_cli_is_unknown(monkeypatch: pytest.MonkeyPatch) -> None: - def explode(*_args: object, **_kwargs: object) -> None: - raise FileNotFoundError("claude") +# --- read_installed: the running harness answers with its own listing ---------------- - monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) - assert read_installed(None) is None +def _listing_ok(argv: list[str], body: str) -> subprocess.CompletedProcess: + completed = subprocess.CompletedProcess[str](argv, 0) + completed.stdout = body + return completed -# --- read_installed: every harness answers with its own listing ---------------------- +def test_the_claude_marker_runs_only_claudes_listing( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The harness running this session is detected from its own marker, + and only its listing command runs.""" + seen: list[list[str]] = [] -def test_a_missing_claude_binary_falls_through_to_codex( + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + seen.append(list(argv)) + return _listing_ok(argv, '[{"name": "magpie-setup", "version": "0.2.0"}]') + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None, {"CLAUDECODE": "1"}) == ("claude", {"magpie-setup": "0.2.0"}) + assert seen == [["claude", "plugin", "list", "--json"]] + + +def test_the_codex_marker_runs_codex_listing_with_its_json_flag( monkeypatch: pytest.MonkeyPatch, ) -> None: - """A missing `claude` binary on another harness is not unknown: that - harness answers with its own listing (issue #1641).""" + """`codex plugin list` prints a terminal summary by default; only + `--json` makes it emit the array the shared parser reads.""" seen: list[list[str]] = [] def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: seen.append(list(argv)) - if argv[0] == "claude": - raise FileNotFoundError("claude") - completed = subprocess.CompletedProcess[str](argv, 0) - completed.stdout = '[{"name": "magpie-setup", "version": "0.3.1"}]' - return completed + return _listing_ok(argv, '[{"name": "magpie-setup", "version": "0.3.1"}]') + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None, {"CODEX_SANDBOX": "1"}) == ("codex", {"magpie-setup": "0.3.1"}) + assert seen == [["codex", "plugin", "list", "--json"]] + + +def test_the_gemini_marker_runs_gemini_listing_with_json_output( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`gemini extensions list` prints plain text by default; only + `-o json` makes it emit the array the shared parser reads.""" + seen: list[list[str]] = [] + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + seen.append(list(argv)) + return _listing_ok(argv, '[{"name": "magpie", "version": "0.2.0"}]') + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) + assert read_installed(None, {"GEMINI_CLI": "1"}) == ("gemini", {"magpie": "0.2.0"}) + assert seen == [["gemini", "extensions", "list", "-o", "json"]] + + +def test_the_innermost_marker_wins_when_two_harnesses_inherit( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A harness started from inside another inherits the outer one's + variables; the innermost candidate answers, and `CLAUDECODE` — which + every child inherits — is checked last.""" + seen: list[list[str]] = [] + + def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: + seen.append(list(argv)) + return _listing_ok(argv, '[{"name": "magpie", "version": "0.2.0"}]') monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) - assert read_installed(None) == {"magpie-setup": "0.3.1"} - assert seen == [["claude", "plugin", "list", "--json"], ["codex", "plugin", "list"]] + harness, _ = read_installed(None, {"CLAUDECODE": "1", "GEMINI_CLI": "1"}) + assert harness == "gemini" + assert seen == [["gemini", "extensions", "list", "-o", "json"]] -def test_no_harness_cli_at_all_is_unknown(monkeypatch: pytest.MonkeyPatch) -> None: - """A machine where none of the listing commands exist stays unknown.""" +def test_no_marker_is_unknown_and_no_listing_runs( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A session under no detected harness stays unknown: nothing is + probed in a fixed order, so no harness can answer for another.""" def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: - raise FileNotFoundError("no harness here") + raise AssertionError("no listing may run without a detected harness") monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) - assert read_installed(None) is None + assert read_installed(None, {}) == (None, None) -def test_a_found_harness_that_fails_is_unknown_not_a_fallthrough( +def test_the_copilot_marker_is_unknown_without_a_listing_cli( monkeypatch: pytest.MonkeyPatch, ) -> None: - """The first binary that exists answers for the machine: a listing it - cannot produce stays unknown rather than borrowing another harness's.""" + """Copilot installs from a repo URL and has no listing CLI: the floor + stays unknown rather than guessing what a listing would print.""" + + def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: + raise AssertionError("copilot has no listing command to run") + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) + assert read_installed(None, {"COPILOT_CLI": "1"}) == (None, None) + + +def test_a_timeout_of_the_running_harness_is_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A `claude` that exists and times out must not borrow another + harness's listing: unknown, and no other harness is ever probed.""" + + def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: + raise subprocess.TimeoutExpired(cmd=["claude"], timeout=20) + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) + assert read_installed(None, {"CLAUDECODE": "1"}) == (None, None) + + +def test_a_refused_binary_of_the_running_harness_is_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A sandbox that refuses to execute the harness's own binary reads as + unknown, not as a missing binary to probe around.""" + + def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: + raise PermissionError("sandbox denies execution") + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) + assert read_installed(None, {"CLAUDECODE": "1"}) == (None, None) + + +def test_a_missing_binary_of_the_running_harness_is_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The marker says Claude Code but `claude` is not on PATH: unknown — + the checker never asks a different harness to answer for it.""" + + def explode(*_args: object, **_kwargs: object) -> subprocess.CompletedProcess: + raise FileNotFoundError("claude") + + monkeypatch.setattr("setup_preflight.cli.subprocess.run", explode) + assert read_installed(None, {"CLAUDECODE": "1"}) == (None, None) + + +def test_a_failed_listing_of_the_running_harness_is_unknown( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A detected harness whose listing exits non-zero stays unknown.""" def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: - if argv[0] == "claude": - raise FileNotFoundError("claude") completed = subprocess.CompletedProcess[str](argv, 1) completed.stdout = "" return completed monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) - assert read_installed(None) is None + assert read_installed(None, {"CLAUDECODE": "1"}) == (None, None) -def test_an_unparsable_listing_from_the_found_harness_is_unknown( +def test_an_unparsable_listing_of_the_running_harness_is_unknown( monkeypatch: pytest.MonkeyPatch, ) -> None: - """A found harness whose listing is not the shared JSON shape reads as - unknown — the checker proposes nothing it cannot read.""" + """A detected harness whose listing is not the shared JSON shape reads + as unknown — the checker proposes nothing it cannot read.""" def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: - if argv[0] == "claude": - raise FileNotFoundError("claude") - completed = subprocess.CompletedProcess[str](argv, 0) - completed.stdout = "magpie-setup 0.3.1 (marketplace)" - return completed + return _listing_ok(argv, "magpie-setup 0.3.1 (marketplace)") monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) - assert read_installed(None) is None + assert read_installed(None, {"CLAUDECODE": "1"}) == (None, None) -def test_a_present_claude_listing_is_not_second_guessed( +def test_the_finding_names_the_running_harness( + project: Path, + capsys: pytest.CaptureFixture[str], monkeypatch: pytest.MonkeyPatch, ) -> None: - """A readable Claude Code listing ends the probe: later harnesses are - never asked, so the established behaviour is unchanged.""" - seen: list[list[str]] = [] + """The `below-floor` finding carries the harness its listing came + from, so the rules can name that harness's own commands.""" + write_lock(project, MARKETPLACE) + monkeypatch.setenv("CLAUDECODE", "1") + monkeypatch.setattr( + "setup_preflight.cli.subprocess.run", + lambda argv, **_kwargs: _listing_ok(argv, '[{"name": "magpie-setup", "version": "0.1.0"}]'), + ) + code, payload = run( + capsys, + "--skill", + "magpie-x", + "--project-root", + str(project), + "--verify-interval-days", + "0", + ) + assert code == 0 + finding = next(f for f in payload["findings"] if f["code"] == "below-floor") + assert finding["facts"]["harness"] == "claude" - def run(argv: list[str], **_kwargs: object) -> subprocess.CompletedProcess: - seen.append(list(argv)) - completed = subprocess.CompletedProcess[str](argv, 0) - completed.stdout = '[{"name": "magpie-setup", "version": "0.2.0"}]' - return completed - monkeypatch.setattr("setup_preflight.cli.subprocess.run", run) - assert read_installed(None) == {"magpie-setup": "0.2.0"} - assert seen == [["claude", "plugin", "list", "--json"]] +def test_a_supplied_listing_reports_no_harness(project: Path, capsys) -> None: + """`--plugin-list` is what the caller already read; the map stands + alone and the finding names no harness.""" + write_lock(project, MARKETPLACE) + listing = project / "plugins.json" + listing.write_text('[{"name": "other", "version": "1.0.0"}]') + _, payload = run( + capsys, + "--skill", + "magpie-x", + "--project-root", + str(project), + "--plugin-list", + str(listing), + ) + finding = next(f for f in payload["findings"] if f["code"] == "below-floor") + assert finding["facts"]["harness"] is None diff --git a/tools/setup-preflight/tests/test_core.py b/tools/setup-preflight/tests/test_core.py index a47807f93..d08341b46 100644 --- a/tools/setup-preflight/tests/test_core.py +++ b/tools/setup-preflight/tests/test_core.py @@ -88,6 +88,17 @@ def test_a_genuinely_missing_plugin_is_a_finding(project: Path) -> None: found = project_findings(project, {"something-else": "9.9.9"}) assert codes(found) == ["below-floor"] assert found[0].facts["missing"] == ["magpie-setup"] + # The listing came from `--plugin-list` here, so no harness is named. + assert found[0].facts["harness"] is None + + +def test_the_floor_finding_names_the_harness_it_measured(project: Path) -> None: + """The finding carries the harness the listing came from, so the rules + can name that harness's own install and update commands.""" + write_lock(project, MARKETPLACE) + found = project_findings(project, {"something-else": "9.9.9"}, "gemini") + assert codes(found) == ["below-floor"] + assert found[0].facts["harness"] == "gemini" def test_being_ahead_of_the_floor_is_never_a_finding(project: Path) -> None: @@ -338,6 +349,19 @@ def test_a_changed_plugin_listing_invalidates_the_cache(project: Path) -> None: assert cached is False and found == [] +def test_a_changed_harness_invalidates_the_cache(project: Path) -> None: + """The below-floor finding names the harness its listing came from, so + a verdict computed under one harness must not be served to another.""" + write_lock(project, MARKETPLACE) + (project / ".apache-magpie-local").mkdir() + first, cached = cached_project_findings(project, {"magpie-setup": "0.1.0"}, "claude") + assert cached is False and codes(first) == ["below-floor"] + assert first[0].facts["harness"] == "claude" + second, cached = cached_project_findings(project, {"magpie-setup": "0.1.0"}, "codex") + assert cached is False and codes(second) == ["below-floor"] + assert second[0].facts["harness"] == "codex" + + def test_a_refreshed_checker_invalidates_the_cache(project: Path) -> None: """`upgrade` replaces the checker's copy; a verdict the old code computed must not outlive it.""" diff --git a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/expected.json b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/expected.json index 56cd059af..86b5931c6 100644 --- a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/expected.json +++ b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/expected.json @@ -1 +1 @@ -{"action": "print-command", "commands": [], "blocks": true} +{"action": "update", "commands": ["gemini extensions update magpie"], "blocks": true, "mention_restart": true} diff --git a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/report.md b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/report.md index aee4b3389..038747fd8 100644 --- a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/report.md +++ b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/report.md @@ -7,5 +7,8 @@ magpie-agent-guard. `.apache-magpie-overrides/` exists and contains `project.md`. -The running agent is Google Gemini CLI. There is no `claude` executable on -PATH. The installed Magpie extension reports version 0.2.0. +The running agent is Google Gemini CLI (`GEMINI_CLI=1`). There is no +`claude` executable on PATH and no Codex session markers in the +environment. The installed Magpie extension reports version 0.2.0: +`gemini extensions list -o json` prints +`[{"name": "magpie", "version": "0.2.0"}]`. diff --git a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/case-meta.json b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/case-meta.json new file mode 100644 index 000000000..03128a3ee --- /dev/null +++ b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/case-meta.json @@ -0,0 +1 @@ +{"tags":["local-smoke","smoke"]} diff --git a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/expected.json b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/expected.json new file mode 100644 index 000000000..d2a0200ed --- /dev/null +++ b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/expected.json @@ -0,0 +1 @@ +{"action": "update", "commands": ["codex plugin update magpie-setup", "codex plugin update magpie-agent-guard"], "blocks": true, "mention_restart": true} diff --git a/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/report.md b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/report.md new file mode 100644 index 000000000..ec6c04446 --- /dev/null +++ b/tools/skill-evals/evals/setup/preflight-floor/fixtures/case-9-below-floor-codex/report.md @@ -0,0 +1,13 @@ + + +`.apache-magpie.lock` contains `method: marketplace`, `url: apache/magpie`, +`min_version: 0.3.0`, and a `plugins` list of magpie-setup and +magpie-agent-guard. + +`.apache-magpie-overrides/` exists and contains `project.md`. + +The running agent is OpenAI Codex CLI (`CODEX_SANDBOX=1`). There is no +`claude` executable on PATH and no Gemini session markers in the +environment. `codex plugin list --json` reports magpie-setup 0.2.0 and +magpie-agent-guard 0.2.0, both from the `apache-magpie` marketplace.