Skip to content

fix(setup-preflight): read the version floor through each harness's own listing - #1673

Open
liwenjie200543 wants to merge 1 commit into
apache:mainfrom
liwenjie200543:fix/preflight-floor-other-harnesses
Open

liwenjie200543 wants to merge 1 commit into
apache:mainfrom
liwenjie200543:fix/preflight-floor-other-harnesses

Conversation

@liwenjie200543

Copy link
Copy Markdown
Contributor

What

read_installed in the setup pre-flight only ever ran claude plugin list --json. On Codex, Gemini CLI or Copilot CLI that binary is missing, the listing collapsed to unknown, and _floor_findings treated 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.md Step 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-list escape hatch behaves exactly as before. Parsing is shared in one _parse_listing helper, so every harness yields the same {name: version} map.

How checked

  • 6 new tests in tools/setup-preflight/tests/test_cli.py: fall-through to Codex when claude is 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.py passes 14/14.
  • One open question for reviewers: I could not verify locally what codex plugin list / gemini extensions list actually 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 suggested preflight-floor eval 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.

@liwenjie200543
liwenjie200543 force-pushed the fix/preflight-floor-other-harnesses branch from c36ec02 to ac258ac Compare October 16, 2026 09:41
@github-actions github-actions Bot added family:setup setup-* skills family:tools tools/* capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) substrate:setup Tool substrate: adopter-side setup state resolved at runtime (lock parsing, floors, fingerprints) labels Oct 16, 2026
…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.
@liwenjie200543
liwenjie200543 force-pushed the fix/preflight-floor-other-harnesses branch from ac258ac to 831d20f Compare October 16, 2026 10:02

@Kaap10 Kaap10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 list is human-readable text (--output-format choices: ["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 list also 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. claude installed globally via npm/brew, but the user is currently working inside a Gemini CLI or Codex session), the sequential loop finds claude on PATH first and queries it. If Claude doesn't have the Magpie plugin installed, it returns None and 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 Code
    • CODEX_SANDBOX or CODEX_THREAD_ID ➔ Codex
    • GEMINI_CLI == "1" or GEMINI_PROJECT_DIR in env ➔ Gemini CLI
    • COPILOT_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): In test_a_missing_claude_binary_falls_through_to_codex, the mock should assert that --json / -o json is present in argv so realistic CLI arguments are verified.
  • CLI help string in build_parser():
    parser.add_argument(
        "--plugin-list",
        metavar="PATH",
        help="`claude plugin list --json` output to use instead of running it ('-' for stdin)",
    )
    Now that listing is harness-agnostic, the help text should say: "harness plugin listing JSON to use instead of running it ('-' for stdin)".

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and claude plugin list --json 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.

        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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]], ...] = (

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actions.

#: 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"]),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-read the diff and check that every change is intentional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capability:platform Framework / agent substrate skills (install, verify, doctor, override, status, setup bootstrap) family:setup setup-* skills family:tools tools/* substrate:setup Tool substrate: adopter-side setup state resolved at runtime (lock parsing, floors, fingerprints)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants