fix(mcp): preserve copilotcli multi-transport types (#1401) by dyoshikawa-claw · Pull Request #1410 · dyoshikawa/rulesync

dyoshikawa-claw · 2026-03-31T07:24:09Z

What is the problem?

PR #1376 added Copilot CLI MCP support, but review findings (#1401) revealed issues:

addTypeField forced all servers to type: "stdio", but Copilot CLI also supports http, sse, and local transports (confirmed via official docs)
Redundant type assertions and eslint-disable in the spread logic
Dead if(global) branch in getSettablePaths
removeTypeField stripped type unconditionally
Missing copilotcli target in rules-processor
No test for http/sse servers passing through correctly
transport enum missing "local" (inconsistent with type enum)
resolveCopilotcliServerType did not handle transport: "local"
Error message hardcoded "stdio" even for local-type servers

How did I fix it?

addTypeField: defaults missing type to "stdio", preserves explicit http/sse/local types, only validates command for stdio/local servers, removes redundant type assertions
removeTypeField: only strips type when it is "stdio"
Removed dead if(global) branch
Added CopilotcliRule and wired copilotcli into rules-processor
Added "local" to transport enum in McpServerSchema for consistency with type enum
Updated resolveCopilotcliServerType to handle transport: "local"
Changed error message to use dynamic ${type} instead of hardcoded "stdio"
Updated JSDoc to reflect the function's actual behavior
Added tests for http/sse passthrough, non-stdio type preservation, and command-less stdio rejection

How was this tested?

4632 tests pass, pnpm cicheck clean

Closes #1401

dyoshikawa-claw · 2026-03-31T08:01:52Z

Reviewed PR #1410: fix(mcp): preserve copilotcli multi-transport types (#1401)

Findings:

[high] transport-based remote MCP servers still break in Copilot CLI conversion
The new logic only treats a server as remote when parsed.type !== "stdio", but the shared MCP schema also allows remote transports via transport ("http" / "sse"). Inputs like { transport: "http", url: "..." } still fall back to "stdio" and raise the missing-command error, so the fix is incomplete for a supported schema shape. The new tests only cover type, not transport.
Refs: src/features/mcp/copilotcli-mcp.ts:31, src/types/mcp.ts:3, src/features/mcp/copilotcli-mcp.test.ts:418
[mid] copilotcli rules support is added without matching gitignore entries
This PR adds copilotcli as a rules target, but gitignore-entries.ts still only registers the MCP config for that target. Users generating .gitignore for --targets copilotcli will not get ignore entries for the rule files now produced by this change, which makes the rollout inconsistent and increases the chance of accidentally committing generated files.
Refs: src/features/rules/rules-processor.ts:296, src/features/rules/copilotcli-rule.ts:10, src/cli/commands/gitignore-entries.ts:140
[low] Documentation is out of sync with the new Copilot CLI behavior
The implementation now adds copilotcli rule support and preserves non-stdio MCP servers, but the docs still describe Copilot CLI as MCP-only and still say Rulesync always injects "type": "stdio". That will mislead users about both rules support and remote MCP handling.
Refs: src/features/rules/rules-processor.ts:296, src/features/mcp/copilotcli-mcp.ts:34, README.md:74, docs/reference/supported-tools.md:13, docs/guide/global-mode.md:5, docs/reference/file-formats.md:72

Security review:

No concrete security findings in this diff.

dyoshikawa-claw · 2026-03-31T10:54:52Z

Reviewed .pr-1410-v2.diff for PR #1410.

Overall: solid follow-up to the earlier review, and I did not find any security vulnerabilities or signs of malicious code. I do think there are still a few merge-blocking correctness/spec issues to address.

Findings

[mid] CopilotcliRule re-enables validation even when the caller explicitly passes validate: false. In src/features/rules/copilotcli-rule.ts:20, fromCopilotRule() hard-codes validate: true, so fromRulesyncRule(), fromFile(), and forDeletion() no longer preserve the original validation mode. That breaks existing opt-out flows and can cause unexpected failures in code paths that intentionally skip validation.
[mid] The implementation now adds copilotcli rule support, but the public support matrix still says copilotcli does not support rules. src/features/rules/rules-processor.ts:78 and src/cli/commands/gitignore-entries.ts:137 clearly treat it as a rules target, while README.md:74 and docs/reference/supported-tools.md:13 still show the rules column as empty. This is a user-facing contract mismatch.
[mid] The Copilot CLI MCP behavior is now multi-transport, but the documented format still says Rulesync always ensures "type": "stdio". src/features/mcp/copilotcli-mcp.ts:30 now preserves or derives http/sse/local, while docs/reference/file-formats.md:77 still documents stdio-only normalization. If multi-transport support is intentional, the docs/spec need to be updated in the same PR.
[low] The diff includes tracked review artifacts: .issue-1401.md, .pr-1408.diff, and .pr-1410.diff. These are not product files, add noise to the repository, and .issue-1401.md already contains statements that are stale relative to the final implementation. Unless there is a specific archival reason to keep them, they should stay out of the PR.

Security review

No security findings.
No signs of backdoors, malicious code, or suspicious dependency changes.

dyoshikawa-claw · 2026-03-31T12:33:05Z

Reviewed PR #1410 and found two issues worth fixing before merge.

#1 [mid] copilotcli rule generation shares file ownership with copilot

Affected: src/features/rules/copilotcli-rule.ts, src/features/rules/rules-processor.ts
CopilotcliRule inherits CopilotRule's paths, so it reads/writes the same .github/copilot-instructions.md and .github/instructions/ files as the existing copilot target. This PR also registers copilotcli as a separate rules target, so generate/import/delete flows are no longer isolated: operating on copilotcli can overwrite or delete files that are also owned by copilot.

#2 [mid] Remote MCP configs no longer fail fast when the endpoint is missing

Affected: src/features/mcp/copilotcli-mcp.ts
In addTypeField(), any server resolved as type: "http" or type: "sse" is accepted immediately and passed through with a type field, but there is no transport-specific validation that url or httpUrl is present. A config like { type: "http" } now silently generates a broken .copilot/mcp-config.json instead of surfacing a clear validation error during generation.

Security review: I did not find a concrete exploitable security issue in this diff, but this change does widen the trust boundary because Rulesync will now preserve external MCP endpoints and local executables for Copilot CLI.

dyoshikawa-claw · 2026-03-31T14:46:26Z

PR #1410 review

#1 [mid] Copilot CLI MCP round-trip is no longer idempotent for transport-based servers

src/features/mcp/copilotcli-mcp.ts:30 resolves { transport: "http" | "sse" } to a synthetic type.
removeTypeField() only strips type when it is "stdio" (src/features/mcp/copilotcli-mcp.ts:104), so importing a generated .copilot/mcp-config.json back into .rulesync/mcp.json changes { transport: "http", url: ... } into { transport: "http", type: "http", url: ... }.
That makes import/generate non-idempotent and introduces source-of-truth churn for users who intentionally use transport-based configs. Consider dropping synthesized non-stdio type on the way back, or normalizing to one canonical shape in both directions.

#2 [mid] httpUrl is accepted but never normalized to the Copilot CLI field name

addTypeField() now allows remote servers with either url or httpUrl (src/features/mcp/copilotcli-mcp.ts:55), but it writes the parsed object back unchanged.
That means a Rulesync config with only httpUrl will generate .copilot/mcp-config.json with httpUrl and no url, even though the documented Copilot CLI format in docs/reference/file-formats.md:68 uses url.
Other adapters already normalize this shape (src/features/mcp/opencode-mcp.ts:156). Please either map httpUrl -> url for Copilot CLI output or add a compatibility test proving httpUrl is supported there.

Security review: I did not find an additional standalone security issue beyond the compatibility and round-trip concerns above.

dyoshikawa

Solid PR overall. The multi-transport support is well-implemented and the test coverage for the MCP changes is thorough. The gitignore multi-target refactor is clean too. A couple of things worth addressing inline — the JSDoc drift and the transport/type enum inconsistency are the main ones. No security concerns found.

dyoshikawa · 2026-04-01T01:45:49Z

+  return "stdio";
+};
+
 /**


The top-level description still says "Adds type: stdio to each MCP server config if not present" but the function now resolves and sets the appropriate transport type (http, sse, local, or stdio). The @throws annotations were updated which is good, but the first line should reflect the broader behavior — something like "Resolves and sets the transport type for each MCP server config."

dyoshikawa · 2026-04-01T01:45:49Z


 export const McpServerSchema = z.looseObject({
-  type: z.optional(z.enum(["stdio", "sse", "http"])),
+  type: z.optional(z.enum(["local", "stdio", "sse", "http"])),


"local" was added to the type enum here, but the transport enum on line 14 still only has ["stdio", "sse", "http"]. If someone writes transport: "local" instead of type: "local", resolveCopilotcliServerType will silently fall through to "stdio" since it only checks for "http" and "sse". Worth keeping these two enums in sync.

dyoshikawa · 2026-04-01T01:45:49Z

      ).rejects.toThrow('MCP server "no-command-server" is missing a command');
    });

+    it("should throw error when stdio server has unknown fields but no command", async () => {


The test name says "unknown fields but no command" but the server config has url and headers which make it look like it should be an http server. The actual point is that without an explicit type/transport, it defaults to stdio and then fails on missing command — but the config is a bit misleading. Either simplify the config to just { unknown_field: "value" } or adjust the test name to clarify the defaulting behavior.

The transport enum was missing the "local" value that was already added to the type enum, which could cause silent fallback to stdio when transport: "local" was specified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ynamic type in error message - Add transport: local to resolveCopilotcliServerType so it no longer silently falls back to stdio - Use the resolved type variable in the missing-command error message instead of hardcoding stdio - Update JSDoc to reflect the function's actual behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dyoshikawa · 2026-04-01T05:17:21Z

@dyoshikawa-claw Thank you!

fix: preserve copilotcli mcp transport types

76e6b8b

fix: handle transport field and add copilotcli gitignore entries

ab80217

fix: align copilotcli rules support and docs

1eda18f

fix: validate remote copilotcli MCP endpoints

e7e3bc3

dyoshikawa reviewed Apr 1, 2026

View reviewed changes

dyoshikawa and others added 2 commits March 31, 2026 19:16

dyoshikawa merged commit f0203b4 into main Apr 1, 2026
10 checks passed

dyoshikawa deleted the fix/issue-1401-copilotcli-multi-transport branch April 1, 2026 05:17

Navigation Menu

Uh oh!

fix(mcp): preserve copilotcli multi-transport types (#1401) - #1410

fix(mcp): preserve copilotcli multi-transport types (#1401)#1410
dyoshikawa merged 6 commits into
mainfrom
fix/issue-1401-copilotcli-multi-transport

dyoshikawa-claw commented Mar 31, 2026 •

edited by dyoshikawa

Loading

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa left a comment

Uh oh!

dyoshikawa Apr 1, 2026

Uh oh!

dyoshikawa Apr 1, 2026

Uh oh!

dyoshikawa Apr 1, 2026

Uh oh!

Uh oh!

dyoshikawa commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dyoshikawa-claw commented Mar 31, 2026 • edited by dyoshikawa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the problem?

How did I fix it?

How was this tested?

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa-claw commented Mar 31, 2026

Uh oh!

dyoshikawa left a comment

Choose a reason for hiding this comment

Uh oh!

dyoshikawa Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

dyoshikawa Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

dyoshikawa Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dyoshikawa commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dyoshikawa-claw commented Mar 31, 2026 •

edited by dyoshikawa

Loading