Repository navigation
fix(setup-preflight): read the version floor through each harness's own listing - #1673
liwenjie200543 wants to merge 1 commit into
Conversation
c36ec02 to
ac258ac
Compare
…wn 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 apache#1641.
ac258ac to
831d20f
Compare
Kaap10
left a comment
There was a problem hiding this comment.
Thanks @liwenjie200543 for working on #1641! Broadening setup-preflight version floor detection across other harnesses is an important improvement.
There are a few functional blockers with the current implementation that prevent it from working in live environments, plus a couple of edge cases to address:
1. Major: Missing JSON flags for gemini and codex listing commands
In plugins/magpie-setup/skills/setup/setup_preflight/cli.py:
HARNESS_LISTINGS: tuple[tuple[str, list[str]], ...] = (
("claude-code", ["claude", "plugin", "list", "--json"]),
("codex", ["codex", "plugin", "list"]),
("gemini", ["gemini", "extensions", "list"]),
)- Gemini CLI: The default output format for
gemini extensions listis human-readable text (--output-formatchoices:["text", "json"], default:"text"). Without-o json(or--output-format json), it prints plain text (e.g.No extensions installed.or text lines). - Codex CLI:
codex plugin listalso prints a formatted terminal summary by default. It requires--json(codex plugin list --json) to emit a JSON array.
Impact: In actual environments, completed.stdout contains plain text. _parse_listing catches json.JSONDecodeError and returns None (unknown). As a result, the floor check continues to be silently skipped on live Codex and Gemini runs. The unit tests in test_cli.py only passed because the mock returned JSON for a command that does not emit JSON in reality.
2. Major: Sequential PATH probing vs detecting the active running harness
In plugins/magpie-setup/skills/setup/setup_preflight/cli.py:
for _harness, argv in HARNESS_LISTINGS:
...- Issue #1641 specifically directed: "Detect the running harness, and read its installed versions with its own listing command".
- When multiple AI CLIs are installed on the same machine (e.g.
claudeinstalled globally via npm/brew, but the user is currently working inside a Gemini CLI or Codex session), the sequential loop findsclaudeonPATHfirst and queries it. If Claude doesn't have the Magpie plugin installed, it returnsNoneand halts, completely ignoring the running Gemini or Codex session. - Magpie already has canonical harness-detection patterns (see
tools/adversarial-review/src/adversarial_review/detect.py) using ambient environment variables:CLAUDECODE == "1"➔ Claude CodeCODEX_SANDBOXorCODEX_THREAD_ID➔ CodexGEMINI_CLI == "1"orGEMINI_PROJECT_DIRin env ➔ Gemini CLICOPILOT_CLI➔ Copilot CLI
The active running harness should be detected first; sequential PATH probing should only be an ambiguous fallback if no active harness environment is detected.
3. Major: Missing preflight-floor eval suite coverage
Issue #1641 asked to:
"Add preflight-floor cases for a below-floor Codex and Gemini install."
tools/skill-evals/evals/setup/preflight-floor/fixtures/case-5-no-claude-cli/report.md currently records the previous behavior where Gemini 0.2.0 was ignored. Updating this fixture or adding dedicated eval cases (and validating with python3 tools/skill-evals/render-preflight-fixtures.py --check) is needed to protect against future regressions.
4. Minor / Nits:
- Copilot CLI: Mentioned in the title and description of #1641 ("setup-preflight: the version floor is never checked on Codex, Gemini CLI or Copilot CLI"), but omitted from
HARNESS_LISTINGS. - Unit test mock fidelity (
test_cli.py): Intest_a_missing_claude_binary_falls_through_to_codex, the mock should assert that--json/-o jsonis present inargvso realistic CLI arguments are verified. - CLI help string in
build_parser():Now that listing is harness-agnostic, the help text should say:parser.add_argument( "--plugin-list", metavar="PATH", help="`claude plugin list --json` output to use instead of running it ('-' for stdin)", )
"harness plugin listing JSON to use instead of running it ('-' for stdin)".
potiuk
left a comment
There was a problem hiding this comment.
Thanks for picking up #1641 — the shared _parse_listing and the tests are a good base, but the probe can answer from the wrong harness, and the Codex and Gemini listings will most likely never parse, so the floor stays unchecked where the issue needs it.
Why an unreadable result is unknown rather than absent. (plugins/magpie-setup/skills/setup/setup_preflight/cli.py:111)
Why an unreadable result is unknown rather than absent. Inside a
sandboxed session the plugin cache is read-denied andclaude plugin list --jsonreturns[]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.
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
The handler treats every OSError and SubprocessError as "binary not on PATH", but that set also covers a claude that exists and times out (TimeoutExpired) or that the sandbox refuses to execute (PermissionError). Both then fall through to the next harness, which contradicts the rule the PR states itself ("the first binary that exists decides the answer") and borrows another harness's listing for an unreadable one. Checked against the PR head: with claude raising TimeoutExpired or PermissionError and codex returning a listing, read_installed(None) returns the Codex map after probing ['claude', 'codex'].
Suggested fix: Fall through only on FileNotFoundError; any other OSError or SubprocessError from a binary that exists returns None. Add a test for each (TimeoutExpired, PermissionError) asserting None and that codex is never probed.
The actions. (plugins/magpie-setup/skills/setup/setup_preflight/cli.py:56)
The actions.
- a floor plugin absent →
claude plugin install <plugin>@apache-magpie;- a floor plugin below
min_version→claude plugin update <plugin>@apache-magpie.Where there is no such CLI, run nothing and print the commands instead.
HARNESS_LISTINGS: tuple[tuple[str, list[str]], ...] = (
("claude-code", ["claude", "plugin", "list", "--json"]),
("codex", ["codex", "plugin", "list"]),
("gemini", ["gemini", "extensions", "list"]),
)
The first binary on PATH is not the running harness. Many maintainers have several harnesses installed, and the floor is then checked against whichever comes first in this tuple. This is not hypothetical: in the sandboxed Claude Code session this review ran in, claude is not on PATH while codex and gemini are, so the Codex listing would answer for a Claude Code session. The finding that follows also proposes the claude plugin install/update commands from step-3, whichever harness's listing produced it, and the _harness id is discarded, so nothing downstream can tell which install was measured. Issue #1641 asks to "detect the running harness, and read its installed versions with its own listing command".
Suggested fix: Detect the running harness first (the signals in install.md Step M2, for example the harness's own environment variables), run only that harness's listing, and return the harness id with the map so the findings name that harness's commands. When the harness cannot be detected, stay unknown rather than probing in a fixed order.
Smaller observations
- See inline comments on
plugins/magpie-setup/skills/setup/setup_preflight/cli.py:58,tools/setup-preflight/tests/test_cli.py:147.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
Contributing guide.
| timeout=20, | ||
| check=False, | ||
| ) | ||
| except (OSError, subprocess.SubprocessError): |
There was a problem hiding this comment.
Why an unreadable result is unknown rather than absent.
| #: 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]], ...] = ( |
| #: 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"]), |
There was a problem hiding this comment.
Run the affected skill's eval suite
| # --- read_installed: every harness answers with its own listing ---------------------- | ||
|
|
||
|
|
||
| def test_a_missing_claude_binary_falls_through_to_codex( |
There was a problem hiding this comment.
Re-read the diff and check that every change is intentional.
What
read_installedin the setup pre-flight only ever ranclaude plugin list --json. On Codex, Gemini CLI or Copilot CLI that binary is missing, the listing collapsed to unknown, and_floor_findingstreated unknown as "say nothing" — so the project's version floor was never reported on any harness other than Claude Code (issue #1641).The fix probes each harness's own listing command in order — the ones
install.mdStep M5 already names (claude plugin list --json,codex plugin list,gemini extensions list) — and lets the first binary that exists answer for the machine. A found-but-unreadable listing stays unknown rather than borrowing another harness's answer, the empty-listing sandbox ambiguity is unchanged, and the--plugin-listescape hatch behaves exactly as before. Parsing is shared in one_parse_listinghelper, so every harness yields the same{name: version}map.How checked
tools/setup-preflight/tests/test_cli.py: fall-through to Codex whenclaudeis missing, no harness CLI at all → unknown, found-but-failed → unknown (no fall-through), unparsable listing → unknown, and a readable Claude listing is not second-guessed.test_cli.pypasses 14/14.codex plugin list/gemini extensions listactually print. If either is not the same JSON shape, that listing reads as unknown (the checker proposes nothing it cannot read) — the safe behavior, and the issue's suggestedpreflight-flooreval cases for below-floor Codex/Gemini installs can follow once the output formats are confirmed.AI assistance: this change was investigated and prepared with AI assistance (issue reproduction analysis, patch, and tests), reviewed and tested locally by me before submission.