fix(kilo): accept a bare MCP toggle entry, and type steps as the number it is - #2448
Merged
Conversation
…er it is
A `{"enabled": false}` entry — how Kilo switches off a server another config
layer defines, from the global config, a marketplace, or a VS Code import —
failed the union that models `mcp`. Because `kilo.jsonc` is the file the rules
feature writes too, and the constructor parses the merged content, that took
down the entire `--targets kilo` run with a ZodError rather than the MCP
feature alone: nothing was written at all. The union gains the toggle arm, and
such an entry round-trips as a canonical server carrying only its disabled
state.
`steps` was declared as a list of step objects, but upstream documents a
positive integer — the cap on agentic iterations before a text-only response is
forced. Importing a real `.kilo/agents/*.md` that sets `steps: 30` therefore
threw, and the tests locked the wrong shape in.
The conversion is split into three named helpers on the way past; it had grown
past the complexity ceiling with the third arm.
The issue's other items are not covered here: the `sandbox` block needs a new
override surface, and global non-root rules need the rules adapter to gain a
global `nonRoot` path plus the shared-write registration that goes with it.
Refs #2417
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The toggle arm landed on the import side only. Generating from a canonical
server that carries no transport — which is exactly what importing a
`{"enabled": false}` entry produces — still emitted
`{type: "local", command: []}`, shadowing the definition the other config
layer holds with one that cannot start, and re-importing that threw on the
empty command array. Such a server now goes back out as the toggle it was, so
the entry round-trips as itself.
The arm is also `strictObject` now rather than `looseObject`. A loose arm
matched any `local` or `remote` entry that happened to carry `enabled` but was
malformed in some other way — a string `command`, say — and silently dropped
its transport instead of failing the way it does today.
`steps` gains the bound upstream documents: a positive integer, nullable so
an author can clear an inherited value.
Refs #2417
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The toggle arm was strict, so a toggle entry carrying any key this adapter does not model aborted the whole --targets kilo run — the failure the arm was added to prevent. Make it loose like every other schema here and refuse only entries carrying a transport key, so a malformed local/remote entry still fails loudly instead of being silently read as a toggle. Also warn when a transport-less canonical server carries fields a toggle cannot keep (args, env, headers), since those are dropped on write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… or halting generate
Three findings from the third review round:
- The toggle arm only refused entries carrying type/command/url, so a
malformed remote entry whose transport key was missing but whose headers
and oauth were intact was read as a toggle and written back as bare
{enabled}, taking an Authorization header and OAuth secrets with it. main
failed loudly on that input. Refuse every field of the two transport
schemas instead.
- A server naming a transport it cannot reach (a type with no command, an
http with no url) was still written as {type: local, command: []} — a
server Kilo cannot start and this adapter throws on re-importing. Skip it
with a warning, and do not leave its tool filters behind.
- Copilot CLI threw on a server that declares no transport, which is exactly
what a Kilo toggle imports as; because generateMcpCore has no per-target
catch, one such entry aborted the whole generate run. Skip it with a
warning; a server that does name a transport but lacks its command or url
is still an error.
Also cover the steps bounds added last round, and scope the new Kilo tests
to a test directory instead of the repository root.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… server it cannot start
The fourth review round found the same defect this PR fixed for Kilo still
present in opencode-mcp.ts, and newly reachable: importing a Kilo toggle now
produces a transport-less canonical server, which OpenCode wrote as
{type: local, command: []} — a server OpenCode cannot start and this adapter
throws on reading back, so one generate broke the next import, silently.
Move the checks into mcp-transport.ts and use them from Kilo, OpenCode, and
Copilot CLI: a server with no transport at all, a local one with no command,
and a remote one with no url are each skipped with a warning, and a skipped
server no longer leaves its tool filters behind. httpUrl and the transport
alias now count as a transport everywhere, not just in Kilo.
Derive the Kilo toggle arm's rejected-key list from the two transport
schemas: a field added to either and forgotten in a hand-written list would
bring back the secret-dropping bug the last round fixed.
Also document the steps migration: the list form earlier Rulesync versions
took is rejected, and the run stops with the offending file named rather
than writing an agent file Kilo would ignore.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…mport
Fifth review round, both halves of the same class this PR has been closing:
- Import: kilo-mcp and opencode-mcp threw on {type: local, command: []},
which is exactly what Rulesync used to write for a transport-less server,
so it sits in real projects. importMcpCore has no per-feature catch, so
one such entry took the whole import run down. Read it as the
transport-less server it is; the write side turns it back into a toggle
(Kilo) or skips it (OpenCode).
- Generate: Copilot CLI still threw for a server naming a transport it
cannot reach, and resolved httpUrl-only, streamable-http and ws servers to
stdio, reporting them as missing a command. Resolve the transport through
the shared module and skip with a warning instead.
Also: ws joins the remote transports (it is reached by url, so classifying
it as local mis-stated why it was skipped), the shared module's comments no
longer describe an OpenCode toggle arm and a Gemini CLI adapter that do not
exist, and the module gets its own boundary tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on by accident
Sixth review round:
- Copilot CLI kept httpUrl and wrote no url, so an httpUrl-only server (what
a Claude Code import produces) was generated as an entry Copilot CLI cannot
connect to — a regression from the previous round, where it at least failed
loudly. Normalize it to url. A ws server is now skipped with a warning too,
the way the Kimi Code adapter skips one: writing it as http left a wss:// url
under a transport Copilot CLI speaks HTTP to.
- Kilo wrote {enabled: true} for any transport-less server, including one
whose canonical config says nothing about being enabled. A toggle overrides
the layer that defines the server, so that switched back on what the user
turned off in the global config or a marketplace. It is skipped unless the
enabled state is stated outright, and importing a toggle now states it.
- Kilo cast the canonical oauth field instead of parsing it, so a value of
another shape was written out and then rejected by this adapter's own
constructor mid-generate. Parse it and drop it with a warning.
- The toggle arm's refine message named itself as the only failure, which is
what a malformed local or remote entry saw. Name all three shapes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seventh review round: - Kilo's tools map is keyed by server name and reaches servers mcp does not list at all, so a filter switching off a dangerous tool of a server another config layer defines has to survive whether or not an entry is written for it. Skipping a transport-less server was taking its filters with it. A server skipped for a broken transport still does. - Skipping a transport-less server also deleted a toggle the file already carried, switching that server back on — the same end state the previous round fixed by not writing enabled: true. Leave an existing toggle exactly as it is, and skip only when there is none. - Copilot CLI reads the url scheme too, so a wss:// server that names no transport is skipped rather than written as http, and httpUrl no longer goes out on the local branch either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eighth review round: - The tools map is keyed by server name and reaches servers mcp does not list, but the import side only ever walked mcp — so a filter written for a server with no entry was dropped on the next import and deleted from the file on the generate after that. Rebuild those filters as transport-less canonical servers, splitting at the underscore the write side joined at, so the key that goes back out is the one that came in. - For the same reason, a server skipped for a broken transport keeps its filters too: a filter turning off a dangerous tool is not the entry's to take away. Kilo and OpenCode now agree on this. - Kilo read the existing kilo.jsonc in one parse, so a single sibling entry it could not read discarded every toggle in the file and switched those servers back on. Parse entry by entry. Look the entry up with Object.hasOwn, so a server named after an Object.prototype member is not mistaken for a toggle already present. - Copilot CLI's WebSocket check now resolves the url the same way the write side does, and matches the scheme case-insensitively. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… reads Ninth review round: - Importing a Kilo toggle, a legacy empty-command entry, or a filter for a server mcp does not list put a server with no command and no url into the shared mcpServers map, so the next generate wrote it to .mcp.json, .cursor/mcp.json and everything else — an entry none of those tools can start. Import them into the tool-scoped kilo.mcpServers / opencode.mcpServers block instead, which only that tool reads, the way the Hermes adapter isolates its own fields. - The orphan-filter reconstruction indexed a plain object by a name taken from the config, so a tools key like constructor_x left the filter unwritten and assigned onto Object itself. Guard with Object.hasOwn, as the sibling lookup already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Kilo MCP factory declared supportsEnabledTools/supportsDisabledTools false, so the processor stripped both fields before the adapter saw them. Kilo's tools map is the same per-tool allow/deny surface OpenCode's is, and this adapter has always imported it — so a filter read out of kilo.jsonc was deleted from it on the next generate, re-enabling a tool the user had turned off, and every filter path this PR built was dead on the write side. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
This was referenced Jul 28, 2026
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
2 participants
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.
Fixes the two outright bugs in #2417 — gaps 1 and 3. Gaps 2, 4 and 5 are left open; see the bottom.
1. A
{"enabled": false}MCP entry took down the whole--targets kilorunKilo types
mcpasRecord<string, Info | { enabled: boolean }>: a bare toggle entry switches off a server another config layer defines — the global config, a marketplace, a VS Code import. Rulesync's union requiredtype, so the entry failed to parse.The damage was wider than the MCP feature.
kilo.jsoncis the file the rules feature also patches, and theKiloMcpconstructor parses the merged content, so a config with one such entry aborted the entire run with a ZodError and nothing was written — including rules, which have nothing to do with it.The union gains the toggle arm. Such an entry imports as a canonical server carrying only
disabled: true, with no transport of its own, and writing it back keeps that layer switched off.2.
stepswas typed as a list of step objectsUpstream documents a positive integer: the cap on agentic iterations before a text-only response is forced (
maxStepsis the deprecated alias). So importing a real.kilo/agents/*.mdwithsteps: 30threw, andkilo: { steps: 30 }in a rulesync subagent was rejected — with the tests locking the wrong shape in.Refactor carried along
convertFromKiloFormatwas a single closure doing tool-map splitting, transport branching and field mapping; adding a third arm pushed it past the repo's complexity ceiling. It is three named helpers now —splitKiloServerTools,kiloServerToRulesync, and the mapping itself — with the three union arms read through one shape, since they differ only bytype.Left open
sandboxblock. Needs a new authoring surface on thekilopermissions override (enabled,network,writable_paths,allowed_hosts), and upstream's scope rule is part of the design: a project config may only tighten, andallowed_hosts/writable_pathsare honoured from the global config only. Worth doing deliberately rather than as a rider.~/.kilo/rules/*.md). Needs the rules adapter to gain a globalnonRootpath and the matchinggetExtraSharedWritePathsregistration, so the files land in the globalkilo.jsoncinstructionsarray. Project scope already works..kilo/rules-{mode}/. A mode dimension the rule model does not have; the issue rightly files it as lower priority.Tests
Two MCP assertions for the toggle entry (disabled and enabled), and the
stepsassertions rewritten onto the number the schema now takes — schema parse and thefromRulesyncSubagentround trip.pnpm cicheckpasses; the MCP and subagents e2e suites pass locally.Refs #2417