From 9f74d3b5303689f5a1ddf4fa6ce9cd8fc3ef597a Mon Sep 17 00:00:00 2001 From: ivklgn Date: Mon, 27 Jul 2026 11:52:32 +0300 Subject: [PATCH 1/2] fix: copilot host wiring targeted surfaces Copilot CLI does not read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `archcore init --agent copilot` wired a Copilot CLI teammate to two things that host ignores, so the hook fired into a session with no archcore tools and no context — silently, with a green install log. MCP config went to .vscode/mcp.json in VS Code shape ("servers" key, "type": "stdio"). Copilot CLI dropped that source in v1.0.37 (github/copilot-cli#3019); it belongs to VS Code alone, a surface copilot-adapter-design.adr explicitly scopes out. The path it moves to is NOT .github/mcp.json either — GitHub's config-dir reference lists that one, but it has never been read as a workspace source (github/copilot-cli#1886, still open; confirmed by the maintainer closing #1291). The only project-level MCP source Copilot CLI reads is the workspace-root .mcp.json, keyed "mcpServers", discovered from the working directory up to the git root. That is the same file claude-code already writes, with the same key and the same bare {command, args} entry, so copilot now shares the standard writer instead of carrying its own. Wiring both hosts converges on one entry in either order — pinned by a test, since a shared target is new here and a duplicating merge would reshape the file on every init. Removing the VS Code writer takes the JSONC corruption policy with it: corruptSkipInstall existed solely because .vscode/mcp.json may carry comments. With no JSONC target left, writeMCPConfig drops the policy parameter and always backs up an unparsable file. Second fix, same silent class: `archcore hooks copilot session-start` emitted Claude's {"hookSpecificOutput": {...}} wrapper. We register the hook under camelCase "sessionStart" — Copilot's native format — whose documented output is a bare top-level additionalContext. Copilot's PascalCase compat mode changes only the INPUT payload, never the output schema, so the wrapper parsed as nothing and the session started blind. Session-start now takes a per-host output shape; Copilot gets the native form (no systemMessage slot on that host, so the connected banner is dropped rather than smuggled into context), every other host keeps the Claude-compatible one. Both shapes are pinned by tests. Verified end to end: init writes .mcp.json + .github/hooks/archcore.json + AGENTS.md and no .vscode/, and the two hosts' session-start outputs differ as intended. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/doctor.go | 7 +- cmd/hooks_claude_code.go | 44 ++++- cmd/hooks_claude_code_test.go | 4 +- cmd/hooks_copilot.go | 2 +- cmd/hooks_copilot_test.go | 77 +++++++- cmd/hooks_cursor.go | 2 +- cmd/hooks_gemini_cli.go | 2 +- cmd/hooks_session_dedup_spec_test.go | 20 +-- cmd/init.go | 2 +- internal/agents/copilot.go | 17 +- internal/agents/copilot_test.go | 7 +- internal/agents/mcp_helpers.go | 92 ++-------- internal/agents/mcp_helpers_test.go | 257 ++++++++++----------------- internal/agents/opencode.go | 2 +- 14 files changed, 266 insertions(+), 269 deletions(-) diff --git a/cmd/doctor.go b/cmd/doctor.go index 3c3d104..4bc85b6 100644 --- a/cmd/doctor.go +++ b/cmd/doctor.go @@ -173,10 +173,11 @@ func convergeMCPForAgent(baseDir string, agent *agents.Agent) (bool, error) { switch agent.ID { case agents.Cursor: return agents.ConvergeCursorMCPJSON(path) - case agents.ClaudeCode, agents.GeminiCLI, agents.RooCode: + // Copilot rides the standard shape: its only project-level MCP source is + // the workspace-root .mcp.json, the same file (and key, and entry) as + // claude-code — see copilotMCPPath. + case agents.ClaudeCode, agents.GeminiCLI, agents.RooCode, agents.Copilot: return agents.ConvergeStandardMCPJSON(path) - case agents.Copilot: - return agents.ConvergeVSCodeMCPJSON(path) default: return false, agent.WriteMCPConfig(baseDir) } diff --git a/cmd/hooks_claude_code.go b/cmd/hooks_claude_code.go index 4b4423b..f3c95c6 100644 --- a/cmd/hooks_claude_code.go +++ b/cmd/hooks_claude_code.go @@ -49,6 +49,28 @@ type hookOutput struct { SystemMessage string `json:"systemMessage,omitempty"` } +// copilotHookOutput is Copilot's native sessionStart response: a bare +// top-level additionalContext, no wrapper and no systemMessage slot +// (GitHub's hooks-configuration reference). Copilot's PascalCase +// "Claude-compatible" mode changes only the INPUT payload shape — the output +// schema is the native one either way — so the hooks config we install +// (camelCase "sessionStart") and this writer must agree, or the context is +// parsed as nothing and the session starts blind. +type copilotHookOutput struct { + AdditionalContext string `json:"additionalContext,omitempty"` +} + +// sessionOutputShape selects how a host wants session-start context framed. +type sessionOutputShape int + +const ( + // shapeClaudeCompat — {"hookSpecificOutput": {...}, "systemMessage": ...}. + // Claude Code's native schema, which Cursor and Gemini CLI also accept. + shapeClaudeCompat sessionOutputShape = iota + // shapeCopilotNative — {"additionalContext": "..."}. + shapeCopilotNative +) + // resolveBaseDir returns the base directory from hook input, falling back to cwd. func resolveBaseDir(input *hookInput) (string, error) { if input.CWD != "" { @@ -82,13 +104,15 @@ func newHooksClaudeCodeCmd(version string) *cobra.Command { } func newSessionStartCmd(version string) *cobra.Command { - return newSessionStartHookCmd("session-start", "Handle SessionStart hook event", version) + return newSessionStartHookCmd("session-start", "Handle SessionStart hook event", version, shapeClaudeCompat) } // --- Hook command factories --- -// newSessionStartHookCmd creates a session-start hook command (shared across agents). -func newSessionStartHookCmd(use, short, version string) *cobra.Command { +// newSessionStartHookCmd creates a session-start hook command (shared across +// agents). The shape argument is per-host and NOT cosmetic: a host that does +// not recognize the wrapper reads no context at all. +func newSessionStartHookCmd(use, short, version string, shape sessionOutputShape) *cobra.Command { return &cobra.Command{ Use: use, Short: short, @@ -104,7 +128,7 @@ func newSessionStartHookCmd(use, short, version string) *cobra.Command { if err != nil { return err } - out, err := handleSessionStartDeduped(baseDir, version, input.dedupKey(), defaultSessionStampDir()) + out, err := handleSessionStartDeduped(baseDir, version, input.dedupKey(), defaultSessionStampDir(), shape) if err != nil { return err } @@ -208,7 +232,7 @@ func sweepExpiredStamps(stampDir string) { // An empty key or stampDir fails open (always emit); a suppressed repeat // returns empty output with nil error — hosts must never see a failing hook // here, just silence. -func handleSessionStartDeduped(baseDir, version, key, stampDir string) ([]byte, error) { +func handleSessionStartDeduped(baseDir, version, key, stampDir string, shape sessionOutputShape) ([]byte, error) { dedup := key != "" && stampDir != "" if dedup { key += "\x00" + baseDir @@ -216,7 +240,7 @@ func handleSessionStartDeduped(baseDir, version, key, stampDir string) ([]byte, return nil, nil } } - out, err := handleSessionStart(baseDir, version) + out, err := handleSessionStart(baseDir, version, shape) if err != nil { if dedup { // Don't let a failed build suppress the retry within the window. @@ -229,8 +253,14 @@ func handleSessionStartDeduped(baseDir, version, key, stampDir string) ([]byte, // --- Session Start Handler (Claude Code adapter) --- -func handleSessionStart(baseDir, version string) ([]byte, error) { +func handleSessionStart(baseDir, version string, shape sessionOutputShape) ([]byte, error) { ctx, docCount := buildSessionContext(baseDir) + if shape == shapeCopilotNative { + // No systemMessage slot on this host, so the connected line is dropped + // rather than smuggled into additionalContext — it is a cosmetic banner + // for the user, not context for the model. + return json.Marshal(copilotHookOutput{AdditionalContext: ctx}) + } output := hookOutput{ HookSpecificOutput: map[string]any{ "hookEventName": "SessionStart", diff --git a/cmd/hooks_claude_code_test.go b/cmd/hooks_claude_code_test.go index f001109..cb8825d 100644 --- a/cmd/hooks_claude_code_test.go +++ b/cmd/hooks_claude_code_test.go @@ -47,7 +47,7 @@ func TestHandleSessionStart_WithDocuments(t *testing.T) { t.Fatal(err) } - data, err := handleSessionStart(base, "v0.1.0") + data, err := handleSessionStart(base, "v0.1.0", shapeClaudeCompat) if err != nil { t.Fatalf("handleSessionStart: %v", err) } @@ -86,7 +86,7 @@ func TestHandleSessionStart_Empty(t *testing.T) { t.Parallel() base := setupArchcoreDir(t) - data, err := handleSessionStart(base, "v0.1.0") + data, err := handleSessionStart(base, "v0.1.0", shapeClaudeCompat) if err != nil { t.Fatalf("handleSessionStart: %v", err) } diff --git a/cmd/hooks_copilot.go b/cmd/hooks_copilot.go index ed3a626..d827d98 100644 --- a/cmd/hooks_copilot.go +++ b/cmd/hooks_copilot.go @@ -10,7 +10,7 @@ func newHooksCopilotCmd(version string) *cobra.Command { Short: "Handle GitHub Copilot hook events", } cmd.AddCommand( - newSessionStartHookCmd("session-start", "Handle Copilot SessionStart hook event", version), + newSessionStartHookCmd("session-start", "Handle Copilot SessionStart hook event", version, shapeCopilotNative), ) return cmd } diff --git a/cmd/hooks_copilot_test.go b/cmd/hooks_copilot_test.go index ad65798..431d892 100644 --- a/cmd/hooks_copilot_test.go +++ b/cmd/hooks_copilot_test.go @@ -17,17 +17,84 @@ func TestRunHooksInstallForAgent_CopilotAlsoInstallsMCP(t *testing.T) { t.Fatalf("runHooksInstallForAgent: %v", err) } - // .vscode/mcp.json should exist with "servers" key. - data, err := os.ReadFile(filepath.Join(base, ".vscode", "mcp.json")) + // The workspace-root .mcp.json should exist with the "mcpServers" key — + // the only project-level MCP source Copilot CLI reads. + data, err := os.ReadFile(filepath.Join(base, ".mcp.json")) if err != nil { - t.Fatalf("ReadFile .vscode/mcp.json: %v", err) + t.Fatalf("ReadFile .mcp.json: %v", err) } var raw map[string]json.RawMessage if err := json.Unmarshal(data, &raw); err != nil { t.Fatalf("Unmarshal: %v", err) } - if _, ok := raw["servers"]; !ok { - t.Error("missing 'servers' in .vscode/mcp.json") + if _, ok := raw["mcpServers"]; !ok { + t.Error("missing 'mcpServers' in .mcp.json") + } +} + +// TestHandleSessionStart_CopilotNativeShape pins the output schema divergence. +// The hooks config we install registers camelCase "sessionStart" — Copilot's +// NATIVE format — whose documented output is a bare top-level +// additionalContext. Emitting Claude's hookSpecificOutput wrapper here parses +// as nothing: the hook exits 0, the user sees no error, and the session simply +// starts with no archcore context. That silence is why this is a test and not +// a comment. +func TestHandleSessionStart_CopilotNativeShape(t *testing.T) { + t.Parallel() + base := setupArchcoreDir(t) + + out, err := handleSessionStart(base, "v0.0.0-test", shapeCopilotNative) + if err != nil { + t.Fatalf("handleSessionStart: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(out, &raw); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if _, ok := raw["hookSpecificOutput"]; ok { + t.Error("hookSpecificOutput is Claude's wrapper — Copilot ignores it entirely") + } + if _, ok := raw["systemMessage"]; ok { + t.Error("Copilot's sessionStart schema has no systemMessage slot") + } + ctx, ok := raw["additionalContext"] + if !ok { + t.Fatal("missing top-level additionalContext") + } + var s string + if err := json.Unmarshal(ctx, &s); err != nil { + t.Fatalf("additionalContext must be a JSON string: %v", err) + } + if s == "" { + t.Error("additionalContext is empty — no context would reach the model") + } +} + +// The other hosts must keep the Claude-compatible wrapper: same context, same +// helper, different frame. A refactor that collapsed both onto one shape would +// break whichever host it did not pick. +func TestHandleSessionStart_ClaudeCompatShapeUnchanged(t *testing.T) { + t.Parallel() + base := setupArchcoreDir(t) + + out, err := handleSessionStart(base, "v0.0.0-test", shapeClaudeCompat) + if err != nil { + t.Fatalf("handleSessionStart: %v", err) + } + var raw map[string]json.RawMessage + if err := json.Unmarshal(out, &raw); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if _, ok := raw["additionalContext"]; ok { + t.Error("top-level additionalContext is Copilot's shape, not Claude's") + } + var wrapper map[string]json.RawMessage + if err := json.Unmarshal(raw["hookSpecificOutput"], &wrapper); err != nil { + t.Fatalf("Unmarshal hookSpecificOutput: %v", err) + } + if _, ok := wrapper["additionalContext"]; !ok { + t.Error("missing additionalContext inside hookSpecificOutput") } } diff --git a/cmd/hooks_cursor.go b/cmd/hooks_cursor.go index d255fb1..f91ab54 100644 --- a/cmd/hooks_cursor.go +++ b/cmd/hooks_cursor.go @@ -10,7 +10,7 @@ func newHooksCursorCmd(version string) *cobra.Command { Short: "Handle Cursor hook events", } cmd.AddCommand( - newSessionStartHookCmd("session-start", "Handle Cursor SessionStart hook event", version), + newSessionStartHookCmd("session-start", "Handle Cursor SessionStart hook event", version, shapeClaudeCompat), ) return cmd } diff --git a/cmd/hooks_gemini_cli.go b/cmd/hooks_gemini_cli.go index 6fe1a64..93e8d36 100644 --- a/cmd/hooks_gemini_cli.go +++ b/cmd/hooks_gemini_cli.go @@ -10,7 +10,7 @@ func newHooksGeminiCLICmd(version string) *cobra.Command { Short: "Handle Gemini CLI hook events", } cmd.AddCommand( - newSessionStartHookCmd("session-start", "Handle Gemini CLI SessionStart hook event", version), + newSessionStartHookCmd("session-start", "Handle Gemini CLI SessionStart hook event", version, shapeClaudeCompat), ) return cmd } diff --git a/cmd/hooks_session_dedup_spec_test.go b/cmd/hooks_session_dedup_spec_test.go index 7e477fa..260516c 100644 --- a/cmd/hooks_session_dedup_spec_test.go +++ b/cmd/hooks_session_dedup_spec_test.go @@ -27,7 +27,7 @@ func TestHandleSessionStart_DedupesBySessionID_Spec(t *testing.T) { stampDir := t.TempDir() // 1. First call for session "s1": full context emitted. - first, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir) + first, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("first call: %v", err) } @@ -43,7 +43,7 @@ func TestHandleSessionStart_DedupesBySessionID_Spec(t *testing.T) { // 2. Repeat for the SAME session: empty output, nil error — hosts must // never see a failing hook here, just silence. - second, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir) + second, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("repeat call must not error: %v", err) } @@ -52,7 +52,7 @@ func TestHandleSessionStart_DedupesBySessionID_Spec(t *testing.T) { } // 3. A DIFFERENT session emits again — dedup is per-session, not global. - other, err := handleSessionStartDeduped(base, "v0.0.0-test", "s2", stampDir) + other, err := handleSessionStartDeduped(base, "v0.0.0-test", "s2", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("other-session call: %v", err) } @@ -67,7 +67,7 @@ func TestHandleSessionStart_EmptyKeyFailsOpen(t *testing.T) { stampDir := t.TempDir() for i := 1; i <= 2; i++ { - out, err := handleSessionStartDeduped(base, "v0.0.0-test", "", stampDir) + out, err := handleSessionStartDeduped(base, "v0.0.0-test", "", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("call %d: %v", i, err) } @@ -89,7 +89,7 @@ func TestHandleSessionStart_UnwritableStampDirFailsOpen(t *testing.T) { stampDir := filepath.Join(parent, "stamps") for i := 1; i <= 2; i++ { - out, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir) + out, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("call %d must stay exit-0 with unwritable stamp dir: %v", i, err) } @@ -104,7 +104,7 @@ func TestHandleSessionStart_ExpiredStampReEmits(t *testing.T) { base := setupArchcoreDir(t) stampDir := t.TempDir() - if _, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir); err != nil { + if _, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat); err != nil { t.Fatal(err) } // Age the stamp beyond the window: the next call must emit again. The @@ -115,7 +115,7 @@ func TestHandleSessionStart_ExpiredStampReEmits(t *testing.T) { t.Fatalf("aging stamp: %v", err) } - out, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir) + out, err := handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatal(err) } @@ -141,7 +141,7 @@ func TestHandleSessionStart_ConcurrentDoubleFire_ExactlyOneEmits(t *testing.T) { for i := range n { wg.Go(func() { <-start - outs[i], errs[i] = handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir) + outs[i], errs[i] = handleSessionStartDeduped(base, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) }) } close(start) @@ -170,7 +170,7 @@ func TestHandleSessionStart_SameSessionDifferentProjectsBothEmit(t *testing.T) { baseB := setupArchcoreDir(t) stampDir := t.TempDir() - outA, err := handleSessionStartDeduped(baseA, "v0.0.0-test", "s1", stampDir) + outA, err := handleSessionStartDeduped(baseA, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("project A: %v", err) } @@ -178,7 +178,7 @@ func TestHandleSessionStart_SameSessionDifferentProjectsBothEmit(t *testing.T) { t.Fatal("project A must emit") } - outB, err := handleSessionStartDeduped(baseB, "v0.0.0-test", "s1", stampDir) + outB, err := handleSessionStartDeduped(baseB, "v0.0.0-test", "s1", stampDir, shapeClaudeCompat) if err != nil { t.Fatalf("project B: %v", err) } diff --git a/cmd/init.go b/cmd/init.go index 4f572a3..865c1c8 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -175,7 +175,7 @@ func newInitCmd() *cobra.Command { } cmd.Flags().StringArrayVar(&agentFlags, "agent", nil, - "non-interactive: initialize and install hooks + MCP config + usage hint for the given agent id (repeatable; e.g. claude-code, cursor, codex-cli)") + "non-interactive: initialize and install hooks + MCP config + usage hint for the given agent id (repeatable; e.g. claude-code, cursor, codex-cli, copilot)") cmd.Flags().StringVar(&projectFlag, "project", "", "project root to initialize (default: current directory; env: ARCHCORE_PROJECT_ROOT)") return cmd diff --git a/internal/agents/copilot.go b/internal/agents/copilot.go index e961bbd..f20117f 100644 --- a/internal/agents/copilot.go +++ b/internal/agents/copilot.go @@ -2,7 +2,20 @@ package agents import "path/filepath" -var copilotMCPPath = filepath.Join(".vscode", "mcp.json") +// copilotMCPPath is the workspace-root .mcp.json — the ONLY project-level MCP +// source Copilot CLI reads. It is deliberately the same file claude-code +// writes: both hosts key on "mcpServers" and accept a bare {command, args} +// stdio entry, so one file serves both and writeMCPConfig merges idempotently. +// +// Not .vscode/mcp.json (what this agent wrote before): Copilot CLI dropped +// that source in v1.0.37 (github/copilot-cli#3019), so it is dead config for +// the CLI and belongs to VS Code alone — a surface copilot-adapter-design.adr +// explicitly scopes out. Not .github/mcp.json either: the config-dir docs list +// it, but it has never been read as a workspace source +// (github/copilot-cli#1886, still open; confirmed by the maintainer closing +// #1291). Copilot CLI discovers .mcp.json from the working directory up to the +// git root, so a repo-root file covers monorepo layouts too. +const copilotMCPPath = ".mcp.json" func copilotAgent() *Agent { return &Agent{ @@ -12,7 +25,7 @@ func copilotAgent() *Agent { return filepath.Join(baseDir, copilotMCPPath) }, WriteMCPConfig: func(baseDir string) error { - return WriteVSCodeMCPJSON(filepath.Join(baseDir, copilotMCPPath)) + return WriteStandardMCPJSON(filepath.Join(baseDir, copilotMCPPath)) }, DetectFn: func(baseDir string) bool { return fileExists(filepath.Join(baseDir, ".github", "copilot-instructions.md")) diff --git a/internal/agents/copilot_test.go b/internal/agents/copilot_test.go index 6c4ea4d..2a94a59 100644 --- a/internal/agents/copilot_test.go +++ b/internal/agents/copilot_test.go @@ -7,8 +7,9 @@ import ( ) // TestCopilotAgent_MCPWiring pins the registry closures for Copilot: a typo in -// the .vscode/mcp.json wiring would otherwise ship green because only the -// shared helper was tested, not the per-agent hookup. +// the .mcp.json wiring would otherwise ship green because only the shared +// helper was tested, not the per-agent hookup. The path itself (and the two +// wrong paths it must never write) is covered in mcp_helpers_test.go. func TestCopilotAgent_MCPWiring(t *testing.T) { t.Parallel() base := t.TempDir() @@ -17,7 +18,7 @@ func TestCopilotAgent_MCPWiring(t *testing.T) { t.Fatal("copilot agent not registered") } - wantPath := filepath.Join(base, ".vscode", "mcp.json") + wantPath := filepath.Join(base, ".mcp.json") if got := a.MCPConfigPath(base); got != wantPath { t.Errorf("MCPConfigPath = %q, want %q", got, wantPath) } diff --git a/internal/agents/mcp_helpers.go b/internal/agents/mcp_helpers.go index 0039e21..f328ec6 100644 --- a/internal/agents/mcp_helpers.go +++ b/internal/agents/mcp_helpers.go @@ -16,29 +16,6 @@ type mcpServerEntry struct { Args []string `json:"args"` } -// vscodeMCPEntry represents an MCP server entry in VS Code format (used by Copilot). -type vscodeMCPEntry struct { - Type string `json:"type"` - Command string `json:"command"` - Args []string `json:"args"` -} - -// corruptPolicy controls what happens when the target file exists but is not -// valid strict JSON. -type corruptPolicy int - -const ( - // corruptBackupAndReset backs the original up as .bak with a visible - // warning and starts fresh (backup-invalid-configs.adr.md). The install - // aborts if the backup itself cannot be written. - corruptBackupAndReset corruptPolicy = iota - // corruptSkipInstall leaves the file untouched and prints manual install - // instructions. Used for JSONC-capable targets (.vscode/mcp.json), where - // "invalid strict JSON" is usually a perfectly valid JSONC config whose - // other MCP servers must not be silently replaced. - corruptSkipInstall -) - // mcpWriteMode controls behavior when an archcore entry already exists. type mcpWriteMode int @@ -56,35 +33,21 @@ const ( // It merges an "archcore" entry under serversKey, round-tripping everything // else (unknown keys, other servers, key order) as opaque RawMessage, and // writes atomically. Returns whether the file was changed. -func writeMCPConfig(filePath, serversKey string, entry any, policy corruptPolicy, mode mcpWriteMode) (bool, error) { +func writeMCPConfig(filePath, serversKey string, entry any, mode mcpWriteMode) (bool, error) { dir := filepath.Dir(filePath) if err := os.MkdirAll(dir, 0o755); err != nil { return false, fmt.Errorf("creating directory %s: %w", dir, err) } - var doc *jsonfile.Doc - switch policy { - case corruptBackupAndReset: - var backedUp bool - var err error - doc, backedUp, err = jsonfile.ReadOrBackup(filePath) - if err != nil { - return false, err - } - if backedUp { - fmt.Println(display.WarnLine(fmt.Sprintf("Corrupted %s backed up, starting fresh", filePath))) - } - default: // corruptSkipInstall - var err error - doc, err = jsonfile.Read(filePath) - if err != nil { - entryJSON, _ := json.Marshal(entry) - fmt.Println(display.WarnLine(fmt.Sprintf( - "%s is not valid strict JSON (VS Code configs may contain comments) — left untouched", filePath))) - fmt.Println(display.HintLine(fmt.Sprintf( - "Add archcore manually under %q: \"archcore\": %s", serversKey, entryJSON))) - return false, nil - } + // A target that exists but is not valid strict JSON is backed up as .bak + // with a visible warning and rewritten fresh (backup-invalid-configs.adr). + // The install aborts if the backup itself cannot be written. + doc, backedUp, err := jsonfile.ReadOrBackup(filePath) + if err != nil { + return false, err + } + if backedUp { + fmt.Println(display.WarnLine(fmt.Sprintf("Corrupted %s backed up, starting fresh", filePath))) } servers := jsonfile.NewDoc() @@ -175,49 +138,32 @@ var ( Command: "archcore", Args: []string{"mcp", "--project", "${workspaceFolder}"}, } - copilotMCPEntry = vscodeMCPEntry{ - Type: "stdio", - Command: "archcore", - Args: []string{"mcp"}, - } ) // WriteStandardMCPJSON writes or merges an archcore entry into a standard // mcpServers JSON config file (used by Claude Code, Gemini CLI, Roo Code). func WriteStandardMCPJSON(filePath string) error { - _, err := writeMCPConfig(filePath, "mcpServers", standardMCPEntry, corruptBackupAndReset, mcpKeepExisting) + _, err := writeMCPConfig(filePath, "mcpServers", standardMCPEntry, mcpKeepExisting) return err } // WriteCursorMCPJSON writes or merges an archcore entry into Cursor's // .cursor/mcp.json (see cursorMCPEntry for why its args differ). func WriteCursorMCPJSON(filePath string) error { - _, err := writeMCPConfig(filePath, "mcpServers", cursorMCPEntry, corruptBackupAndReset, mcpKeepExisting) + _, err := writeMCPConfig(filePath, "mcpServers", cursorMCPEntry, mcpKeepExisting) return err } -// WriteVSCodeMCPJSON writes or merges an archcore entry into a VS Code-style -// MCP config file (uses "servers" key + "type": "stdio"), used by GitHub Copilot. -func WriteVSCodeMCPJSON(filePath string) error { - _, err := writeMCPConfig(filePath, "servers", copilotMCPEntry, corruptSkipInstall, mcpKeepExisting) - return err -} - -// ConvergeStandardMCPJSON, ConvergeCursorMCPJSON, and ConvergeVSCodeMCPJSON -// are the doctor --fix counterparts of the writers above: an existing -// archcore entry that drifted from the desired shape (e.g. a Cursor config -// written before --project ${workspaceFolder} existed) is updated in place. -// Foreign servers and unknown keys stay untouched. They report whether the -// file changed. +// ConvergeStandardMCPJSON and ConvergeCursorMCPJSON are the doctor --fix +// counterparts of the writers above: an existing archcore entry that drifted +// from the desired shape (e.g. a Cursor config written before +// --project ${workspaceFolder} existed) is updated in place. Foreign servers +// and unknown keys stay untouched. They report whether the file changed. func ConvergeStandardMCPJSON(filePath string) (bool, error) { - return writeMCPConfig(filePath, "mcpServers", standardMCPEntry, corruptBackupAndReset, mcpConverge) + return writeMCPConfig(filePath, "mcpServers", standardMCPEntry, mcpConverge) } func ConvergeCursorMCPJSON(filePath string) (bool, error) { - return writeMCPConfig(filePath, "mcpServers", cursorMCPEntry, corruptBackupAndReset, mcpConverge) -} - -func ConvergeVSCodeMCPJSON(filePath string) (bool, error) { - return writeMCPConfig(filePath, "servers", copilotMCPEntry, corruptSkipInstall, mcpConverge) + return writeMCPConfig(filePath, "mcpServers", cursorMCPEntry, mcpConverge) } diff --git a/internal/agents/mcp_helpers_test.go b/internal/agents/mcp_helpers_test.go index 27843b7..4462380 100644 --- a/internal/agents/mcp_helpers_test.go +++ b/internal/agents/mcp_helpers_test.go @@ -170,134 +170,6 @@ func TestWriteStandardMCPJSON_CreatesDirs(t *testing.T) { } } -func TestWriteVSCodeMCPJSON_NewFile(t *testing.T) { - t.Parallel() - base := t.TempDir() - filePath := filepath.Join(base, ".vscode", "mcp.json") - - if err := WriteVSCodeMCPJSON(filePath); err != nil { - t.Fatalf("WriteVSCodeMCPJSON: %v", err) - } - - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("ReadFile: %v", err) - } - - var raw map[string]json.RawMessage - if err := json.Unmarshal(data, &raw); err != nil { - t.Fatalf("Unmarshal: %v", err) - } - - var servers map[string]json.RawMessage - if err := json.Unmarshal(raw["servers"], &servers); err != nil { - t.Fatalf("Unmarshal servers: %v", err) - } - - if _, ok := servers["archcore"]; !ok { - t.Error("missing 'archcore' in servers") - } - - var entry struct { - Type string `json:"type"` - Command string `json:"command"` - Args []string `json:"args"` - } - if err := json.Unmarshal(servers["archcore"], &entry); err != nil { - t.Fatalf("Unmarshal entry: %v", err) - } - if entry.Type != "stdio" { - t.Errorf("type = %q, want %q", entry.Type, "stdio") - } - if entry.Command != "archcore" { - t.Errorf("command = %q, want %q", entry.Command, "archcore") - } - if len(entry.Args) != 1 || entry.Args[0] != "mcp" { - t.Errorf("args = %v, want [mcp]", entry.Args) - } -} - -func TestWriteVSCodeMCPJSON_Idempotent(t *testing.T) { - t.Parallel() - base := t.TempDir() - filePath := filepath.Join(base, ".vscode", "mcp.json") - - if err := WriteVSCodeMCPJSON(filePath); err != nil { - t.Fatalf("first call: %v", err) - } - if err := WriteVSCodeMCPJSON(filePath); err != nil { - t.Fatalf("second call: %v", err) - } - - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatalf("ReadFile: %v", err) - } - var raw map[string]json.RawMessage - if err := json.Unmarshal(data, &raw); err != nil { - t.Fatalf("Unmarshal: %v", err) - } - var servers map[string]json.RawMessage - if err := json.Unmarshal(raw["servers"], &servers); err != nil { - t.Fatalf("Unmarshal servers: %v", err) - } - - if len(servers) != 1 { - t.Errorf("expected 1 server entry, got %d", len(servers)) - } -} - -func TestWriteVSCodeMCPJSON_MergesExisting(t *testing.T) { - t.Parallel() - base := t.TempDir() - filePath := filepath.Join(base, ".vscode", "mcp.json") - - if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil { - t.Fatalf("MkdirAll: %v", err) - } - - existing := map[string]any{ - "servers": map[string]any{ - "other-tool": map[string]any{ - "type": "stdio", - "command": "other-tool", - "args": []string{"serve"}, - }, - }, - } - data, err := json.MarshalIndent(existing, "", " ") - if err != nil { - t.Fatalf("MarshalIndent: %v", err) - } - if err := os.WriteFile(filePath, data, 0o644); err != nil { - t.Fatalf("WriteFile: %v", err) - } - - if err := WriteVSCodeMCPJSON(filePath); err != nil { - t.Fatalf("WriteVSCodeMCPJSON: %v", err) - } - - result, err2 := os.ReadFile(filePath) - if err2 != nil { - t.Fatalf("ReadFile: %v", err2) - } - var raw map[string]json.RawMessage - if err := json.Unmarshal(result, &raw); err != nil { - t.Fatalf("Unmarshal: %v", err) - } - var servers map[string]json.RawMessage - if err := json.Unmarshal(raw["servers"], &servers); err != nil { - t.Fatalf("Unmarshal servers: %v", err) - } - - if _, ok := servers["other-tool"]; !ok { - t.Error("existing 'other-tool' was lost during merge") - } - if _, ok := servers["archcore"]; !ok { - t.Error("missing 'archcore' after install") - } -} - func TestWriteStandardMCPJSON_InvalidJSON(t *testing.T) { t.Parallel() base := t.TempDir() @@ -338,37 +210,6 @@ func TestWriteStandardMCPJSON_InvalidJSON(t *testing.T) { } } -// TestWriteVSCodeMCPJSON_JSONCLeftUntouched pins the corruptSkipInstall policy -// for VS Code targets: a JSONC file (valid for VS Code, invalid strict JSON) -// must not be backed up or replaced — the user's other MCP servers survive and -// the install degrades to manual instructions with a nil error. -func TestWriteVSCodeMCPJSON_JSONCLeftUntouched(t *testing.T) { - t.Parallel() - base := t.TempDir() - filePath := filepath.Join(base, ".vscode", "mcp.json") - if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil { - t.Fatal(err) - } - original := "// user comment\n{\"servers\": {\"other\": {\"type\": \"stdio\", \"command\": \"other\"}}}\n" - if err := os.WriteFile(filePath, []byte(original), 0o644); err != nil { - t.Fatal(err) - } - - if err := WriteVSCodeMCPJSON(filePath); err != nil { - t.Fatalf("JSONC file must not fail the install: %v", err) - } - data, err := os.ReadFile(filePath) - if err != nil { - t.Fatal(err) - } - if string(data) != original { - t.Errorf("JSONC file must be left byte-identical:\ngot:\n%s\nwant:\n%s", data, original) - } - if _, err := os.Stat(filePath + ".bak"); !os.IsNotExist(err) { - t.Error("no .bak may be created for a JSONC file") - } -} - // TestWriteStandardMCPJSON_NullServersSection pins the `"mcpServers": null` // handling (previously a nil-map assignment panic). func TestWriteStandardMCPJSON_NullServersSection(t *testing.T) { @@ -448,3 +289,101 @@ func TestWriteStandardMCPJSON_BackupWriteFailureAborts(t *testing.T) { t.Error("original corrupted file must stay untouched when backup fails") } } + +// --- Copilot MCP path ------------------------------------------------------- +// +// Copilot CLI reads exactly one project-level MCP source: the workspace-root +// .mcp.json, keyed "mcpServers". These tests pin that, because the two paths +// this agent must NOT use are both plausible-looking and both wrong: +// .vscode/mcp.json (dropped upstream in v1.0.37, github/copilot-cli#3019) and +// .github/mcp.json (listed in GitHub's config-dir docs but never read — +// github/copilot-cli#1886). Either one silently yields a Copilot session with +// no archcore tools, which is invisible until a user trips it. + +func TestCopilotAgent_WritesWorkspaceRootMCPJSON(t *testing.T) { + t.Parallel() + base := t.TempDir() + agent := copilotAgent() + + want := filepath.Join(base, ".mcp.json") + if got := agent.MCPConfigPath(base); got != want { + t.Errorf("MCPConfigPath = %q, want %q", got, want) + } + if err := agent.WriteMCPConfig(base); err != nil { + t.Fatalf("WriteMCPConfig: %v", err) + } + + for _, dead := range []string{ + filepath.Join(base, ".vscode", "mcp.json"), + filepath.Join(base, ".github", "mcp.json"), + } { + if _, err := os.Stat(dead); !os.IsNotExist(err) { + t.Errorf("%s must not be written — Copilot CLI does not read it", dead) + } + } + + data, err := os.ReadFile(want) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if _, ok := raw["servers"]; ok { + t.Error(`"servers" is the VS Code key; Copilot CLI reads "mcpServers"`) + } + var servers map[string]json.RawMessage + if err := json.Unmarshal(raw["mcpServers"], &servers); err != nil { + t.Fatalf("Unmarshal mcpServers: %v", err) + } + var entry struct { + Command string `json:"command"` + Args []string `json:"args"` + } + if err := json.Unmarshal(servers["archcore"], &entry); err != nil { + t.Fatalf("Unmarshal entry: %v", err) + } + if entry.Command != "archcore" || len(entry.Args) != 1 || entry.Args[0] != "mcp" { + t.Errorf("entry = %+v, want {archcore [mcp]}", entry) + } +} + +// Copilot and claude-code deliberately target the SAME file. Wiring both must +// converge on one archcore entry, in either order — if it ever duplicated or +// fought, a repo wired for both hosts would flip shape on every init. +func TestCopilotAndClaudeCodeShareOneMCPEntry(t *testing.T) { + t.Parallel() + for _, order := range [][]*Agent{ + {copilotAgent(), claudeCodeAgent()}, + {claudeCodeAgent(), copilotAgent()}, + } { + base := t.TempDir() + if order[0].MCPConfigPath(base) != order[1].MCPConfigPath(base) { + t.Fatalf("expected a shared MCP path, got %q and %q", + order[0].MCPConfigPath(base), order[1].MCPConfigPath(base)) + } + for _, a := range order { + if err := a.WriteMCPConfig(base); err != nil { + t.Fatalf("%s WriteMCPConfig: %v", a.ID, err) + } + } + + data, err := os.ReadFile(filepath.Join(base, ".mcp.json")) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + var servers map[string]json.RawMessage + if err := json.Unmarshal(raw["mcpServers"], &servers); err != nil { + t.Fatalf("Unmarshal mcpServers: %v", err) + } + if len(servers) != 1 { + t.Errorf("%s then %s: expected 1 server entry, got %d", + order[0].ID, order[1].ID, len(servers)) + } + } +} diff --git a/internal/agents/opencode.go b/internal/agents/opencode.go index ba1c281..68edfa2 100644 --- a/internal/agents/opencode.go +++ b/internal/agents/opencode.go @@ -34,6 +34,6 @@ func writeOpenCodeMCPConfig(baseDir string) error { _, err := writeMCPConfig(filepath.Join(baseDir, "opencode.json"), "mcp", openCodeMCPEntry{ Type: "local", Command: []string{"archcore", "mcp"}, - }, corruptBackupAndReset, mcpKeepExisting) + }, mcpKeepExisting) return err } From c048cb843c307a9b9904a5c81c38dabeac502ab7 Mon Sep 17 00:00:00 2001 From: ivklgn Date: Mon, 27 Jul 2026 13:30:18 +0300 Subject: [PATCH 2/2] ci: run vet and tests on pull requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `go test ./...` ran in exactly one place: release.yml, on a tag push. So a broken test first surfaced during a release, with the tag already pushed and the only remedy a follow-up tag. Nothing gated a PR — the green checks on one were CodeQL alone, which says nothing about whether the code works. Same three commands the release already trusts, moved to where they can still cheaply say no. Formatting runs first because it is the cheapest signal and the one most likely to differ between a contributor's editor and the repo; gofmt is checked rather than applied so CI never rewrites a contributor's tree. Concurrency cancels superseded runs on branches but not on main, where every commit should keep a result of its own. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 50 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 .github/workflows/test.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..d924ccb --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,50 @@ +name: Test + +# The release workflow already runs `go test ./...`, but only on a tag — so a +# broken test first surfaced during a release, with the tag already pushed. +# This moves the same gate to the point where it can still cheaply say no. + +on: + pull_request: + push: + branches: [main] + workflow_dispatch: + +permissions: + contents: read + +# A force-push supersedes the previous run; keep the newest, drop the rest. +# main is exempt: every commit there should keep a result of its own. +concurrency: + group: test-${{ github.ref }} + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} + +jobs: + test: + name: vet + test + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version-file: go.mod + + # Formatting first: it is the cheapest signal and the one most likely to + # differ between a contributor's editor and the repo. + - name: Check formatting + run: | + unformatted=$(gofmt -l .) + if [ -n "$unformatted" ]; then + echo "::error::gofmt would rewrite these files; run 'gofmt -w .'" + echo "$unformatted" + exit 1 + fi + + - name: Vet + run: go vet ./... + + - name: Test + run: go test ./...