Repository navigation
[AI-1489] Pin the vendor-capable flows schema across every driver projection - #388
Merged
Merged
Conversation
…ojection
Reviewer choice is meant to be a property of the request, not of whichever
harness is driving. Nothing enforced that: registration is FOUR mechanisms, not
one. Six harnesses (Cursor, Copilot, Gemini, Kiro, OpenCode, Antigravity)
converge on one JSON writer and differ only by a shape; Codex writes TOML through
a separate engine with its own ownership ledger; Claude Code loads a
hand-maintained static kcap/.mcp.json; and Pi gets a hard-coded server list
inside an embedded TypeScript bridge.
The existing per-harness tests each assert Contains("kcap-flows") in isolation —
that a server by that name was written, not that it resolves to the same
executable and therefore the same schema. A harness whose registration drifts to
a different command leaves a caller believing it named a reviewer when it sent
nothing.
Adds a conformance suite covering:
- vendor is an optional string on both START tools, and on neither of the six
follow-up tools (the applied vendor is pinned at start; a vendor there would
be ignored or an incoherent mid-run switch);
- the vendor/model DESCRIPTIONS carry what the schema cannot — that omitting
vendor takes the server default, that the token is canonical lowercase, that
there is no silent fallback, and that model requires vendor. This is the only
mechanism by which a driver LLM learns to pass the parameter, so a correct
schema with a silent description produces exactly the failure the contract
exists to prevent;
- all nine driver projections resolve to the same `kcap mcp flows`, driven
through the real writers rather than asserted against the descriptor;
- Pi's bridge still lists flows in its literal — it discovers tools at runtime,
so dropping it there is silent;
- the two independent copies of the server list (KcapMcpServers vs
KcapMcpRegistry) agree, and every canonical server resolves as an allowlist
entry. Nothing kept these in sync.
Also pins the hand-written harness table against VendorSelection.KnownVendorFlags
(now internal), so a tenth installable target fails here instead of quietly
being uncovered — no enumeration of supported harnesses exists in production
code, the list is spread across four separate string arrays.
Mutation-tested: dropping vendor from start_flow, drifting KcapMcpRegistry's
args, removing flows from Pi's literal, adding a tenth vendor flag, and changing
the canonical flows args each fail their assertion. The last one is instructive —
all seven generated arms fail while the two static hand-maintained files pass,
which is exactly the generated-vs-static drift this is for.
Scratch dirs live under the assembly output, not the system temp root: on macOS
/var is a symlink and CodexConfigToml's path guard rejects any symlinked
component, so a temp-rooted Codex registration silently returns Failed. (That is
also why the pre-existing CodexConfigTomlTests fail locally on macOS.)
26/26. Full Tests.Unit: 42 failures, all pre-existing on macOS and none in this
suite.
PR Summary by QodoPin vendor-capable flows schema across driver projections
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Class summary is overly verbose
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…table Codex review round 1, and the P1 defeated the suite's central claim. The table hard-coded KcapMcpServers.ForCursor and the expected McpConfigShape, while the real choices are wired independently in SetupCommand and each PluginCommand installer. So changing a real arm to omit kcap-flows, use the wrong subset, or use the wrong shape left every projection test green — the test kept invoking its own correct reconstruction. The Codex arm had the same hole, calling RegisterKcapMcpServers directly rather than the install path. Each arm now runs PluginCommand.HandleAsync against a FakeUserHome, seeding the same installed-but-stale state its own PluginCommand*Tests use so `--if-installed` takes the refresh branch. Codex is flagged BareInstall — it installs unconditionally and needs a planted plugin root. Proof it now bites: dropping kcap-flows from the production ForCursor subset fails six arms, where before it failed none. Also from round 1: - argv comparisons were UNORDERED, so ["flows","mcp"] passed while launching nothing. Ordered in both the projection assertion and the two-list check. - the drift check only ran canonical -> registry, so a registry-only server stayed allowlistable-but-never-registered with everything green — one of the exact failure modes the suite claims to prevent. Now compares both name sets (KcapMcpRegistry.AllIds is new) and every server's args. - `model` was pinned by prose only, so retyping it to boolean or promoting it into Required — which would break every caller relying on the vendor's default model — still passed. Type and non-requiredness now pinned, mirroring vendor. The coverage tripwire also stops matching on a display name split on whitespace (which could match by accident) and matches on the flag each arm actually drives. Every fix mutation-tested: production dropping flows, a registry-only server, reversed argv order, and model promoted to Required each fail. 27/27. Full Tests.Unit unchanged at 42 pre-existing macOS failures, none mine.
…ontainment Codex review round 2. P1 — the SetupCommand route was outside the gate. `kcap setup` builds its own six Register*Mcp delegates, duplicating the (subset, shape, marker) tuple that PluginCommand also spells out. Mutating SetupCommand.RegisterCopilotMcp to drop flows or use a divergent shape left every installer-driven arm green: a user could get a different tool surface depending on whether they ran `kcap plugin install` or `kcap setup`. Fixed structurally rather than by testing both routes. The tuple now lives once, in HarnessMcpProjections, and both call sites consume it — there is no longer a second definition to diverge. Dropping flows from the shared Copilot projection now fails 2 tests; giving it the wrong shape fails 1. P1 — path containment was broken in three of seven arms, and the test could therefore read and rewrite a developer's REAL harness config, or pass against a pre-existing entry. The Gemini arm cleared GEMINI_HOME, a name GeminiPaths does not read (it honours GEMINI_CLI_HOME); the Codex arm cleared nothing while CodexPaths still gives ambient CODEX_HOME precedence; OpenCode cleared OPENCODE_CONFIG_DIR but left its XDG_CONFIG_HOME fallback live. Every known override is now cleared for every arm — a per-arm list is exactly what was wrong, so there is no per-arm list. Codex also now passes --skip-codex-network-access; a schema test has no business rewriting profile network config. P2 — the status assertion was satisfied by the fallback. FormatStatusResponse catches formatter exceptions and returns the raw JSON body, so "contains claude" passed even if formatting failed entirely. Now asserts the rendered labels and that no raw JSON survives. Mutation-testing that fix found something else: the audit rendering is TRIPLICATED across FormatRoundResponse, FormatStatusResponse and FormatPolledRoundResult, and my first mutant hit the wrong copy. The polled path is the one an agent reads on nearly every flow, and it had no coverage at all — added, and both formatter mutants now fail. 34/34. Full Tests.Unit unchanged at 42 pre-existing macOS failures, none mine.
Codex review round 3. I added HarnessMcpProjection.Unregister and then left it
unused — the six PluginCommand remove paths still hard-coded shape and marker,
and Kiro's "is the MCP half already installed?" probe constructed
`new McpMarker("kiro")` directly. So the single-source claim was only half true:
changing a projection made new installs write under one ownership tuple while
uninstall looked under the old one (stranding owned entries kcap could no longer
see) and Kiro's refresh read an existing install as absent.
All six removals now go through the projection, and OwnsAnything moves the probe
there for the same reason the marker name is derived rather than passed: a probe
reading a different tuple than the writer is the same bug in a third place.
`new McpMarker(` no longer appears in PluginCommand at all.
Pinned by a per-harness register -> probe -> unregister round-trip asserting the
config is left with no kcap entries. Mutation-tested by making Unregister use a
different marker name: all six fail.
41/41. Full Tests.Unit failure set byte-identical to the 42-item pre-existing
macOS baseline.
Qodo #3. The two bundled static configs were covered by an [Arguments] test the tripwire could not see, and both reduce to `--codex` / `--claude` there — so deleting the Codex-plugin arm left `--codex` green while one of two INDEPENDENT Codex registration mechanisms went untested. The bundled configs are now a list the coverage assertion can read, compared against what is actually shipped in kcap/. A third bundled config, or a deleted arm, fails. Mutation-tested by removing the .codex-mcp.json entry. 42/42.
Qodo #1, partially taken. The comments explain why each assertion is shaped the way it is — which is load-bearing here, since three of this PR's review findings were tests that passed vacuously and the rationale is what stops the next reader simplifying them back. That stays, and it matches the surrounding code (SingleFlightRefresh, McpConfigShape, KcapMcpRegistry all document intent at length). What was genuinely historical rather than explanatory is gone: which review round found what, what my earlier attempts did, and past-tense accounts of defects that no longer exist. Rewritten as present-tense reasons. 42/42.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[AI-1489] Pin the vendor-capable flows schema across every driver projection
Leg 1 of AI-1489 — §6 "Schema and registration conformance" plus clarification B's descriptor half. No spend, no reviewers launched. The live crossings and the Copilot routing leg are separate and still blocked on AI-1527.
The gap
Reviewer choice is meant to be a property of the request, not of whichever harness is driving. Nothing enforced that, because registration was four mechanisms and two independent definitions:
McpConfigShapekcap/.mcp.json…and each of the six JSON harnesses had its
(subset, shape, marker)tuple written out twice — once inPluginCommand, once inSetupCommand's installer delegates — with nothing tying them together. A user could get a different tool surface depending on whether they rankcap plugin installorkcap setup.The existing per-harness tests each assert
Contains("kcap-flows")in isolation: that a server by that name was written, not that it resolves to the same executable and therefore the same schema.What changed
HarnessMcpProjections(new,Capacitor.Cli.Core/Mcp/) holds each harness's(subset, shape, marker)once.PluginCommand's six register paths, its six remove paths, Kiro's install probe, and all sixSetupCommanddelegates now consume it.new McpMarker(no longer appears inPluginCommandat all. The divergence is unrepresentable rather than merely detected.VendorSelection.KnownVendorFlagswidenedprivate→internalso the conformance table can pin itself against it.What the suite pins
vendoris an optional string on both START tools, and on neither of the six follow-up tools — the applied vendor is pinned at start, so a vendor on a follow-up is either silently ignored or an incoherent mid-run switch.modelis pinned structurally too (string, not required).vendortakes the server default, that the token is canonical lowercase, that there is no silent fallback, and thatmodelrequiresvendor. The description is the only mechanism by which a driver LLM learns to pass the parameter.kcap mcp flows, driven through the real installers (PluginCommand.HandleAsyncagainst aFakeUserHome, each arm seeding the same installed-but-stale state its own tests use), with ordered argv comparison.flows— it discovers tools at runtime, so dropping it from that literal is silent.KcapMcpServers(registration) andKcapMcpRegistry(allowlist resolution, and the recursion guard's authority) were maintained separately.Tripwires
The harness table is hand-written because no enumeration of supported harnesses exists in production code — the list is spread across four string arrays. So it is pinned against
VendorSelection.KnownVendorFlags, and the bundled static configs are pinned against what is actually shipped inkcap/. A tenth harness, or a deleted arm, fails here rather than going quietly uncovered.Verification
Every fix mutation-tested. These each fail: dropping
vendorfromstart_flow; promotingmodelto required; driftingKcapMcpRegistry's args; a registry-only server; removingflowsfrom Pi's literal; adding a tenth vendor flag; reversing the canonical argv order; dropping flows from the shared Copilot projection; giving Copilot the wrong shape; makingUnregisteruse a different marker; deleting a bundled-config arm; and removing the audit rendering from either formatter.Three earlier versions of these tests passed vacuously and were caught by mutation rather than by review — the reconstructed projection table, the dynamic-path ordering test, and the status assertion satisfied by the formatter's raw-JSON fallback.
Review: codex clean at
0417cf5after 4 rounds (2 P1s, 5 P2s). Qodo: 4 findings, 2 already fixed by the codex rounds, 1 fixed here, 1 answered.Tests: 42/42 in this suite. Full
Tests.Unitfailure set byte-identical to the 42-item pre-existing macOS baseline (CodexConfigTomlTestsand the uninstall paths fail locally because/varis a symlink andCodexConfigToml's path guard rejects symlinked components — unrelated to this change; this suite's scratch dirs sit under the assembly output for that reason).