feat(cli)!: remove mcp-server mode and add scan foundations (Phase 0) by appleboy · Pull Request #12 · go-authgate/agent-scanner

appleboy · 2026-05-23T09:42:46Z

Summary

Phase 0 of the agent-scan → Go port plan: removes the experimental mcp-server / install-mcp-server commands (matching the upstream Python project, which deleted them) and lays the shared foundations later phases build on — a skip-result code, a serverUrl config alias, and a "valid-but-no-MCP" config type. Net effect is ~1300 lines deleted (the internal/mcpserver/ package) and ~207 lines of foundation code + tests added.

AI Authorship

No AI was used in this PR
AI was used. Details:
- Tool / model: Claude Code (Opus 4.7), executing Phase 0 of plan.md
- AI-authored files: all files in this PR
- Human line-by-line reviewed: ⚠️ none yet — author has not reviewed line-by-line. Relying on the test suite + reviewer. Reviewers should treat every changed file as needing close attention.

Change classification

Leaf node (local impact)
Core code — touches internal/models/ (zero-dependency domain types consumed across discovery, inspect, pipeline, output, upload) and the config parser. A bug here propagates system-wide.

Plan reference

Phase 0 of plan.md ("Shared foundations + mcp-server removal"). Scope decisions:

mcp-server & install-mcp-server removed entirely (delete commands + internal/mcpserver/), matching agent-scan.
Add failure/skip code X010 + skipped_by_runtime_config category → code map.
Accept serverUrl as an alias for url in server config parsing.
Empty / no-MCP config files parse clean (new ConfigWithoutMCP, ConfigType()=="no_mcp") instead of being treated as unknown/error.

Deviation from plan (both deliberate, approved):

Plan said "go-sdk dep stays — mcpclient uses it." That assumption was wrong: nothing imports go-sdk (mcpclient implements MCP/JSON-RPC directly); it was used only by the deleted mcpserver. Ran go mod tidy, removing go-sdk + 6 transitive deps.
Plan named server.go for the serverUrl alias; the actual config-file parse path is ServerConfigJSON in config.go, so the alias lives there and in a new RemoteServer.UnmarshalJSON (for direct decode). Canonical url wins when both are present.

Verification

Unit tests — errors_test.go (full category→code map incl. X010), config_test.go (serverUrl alias on both paths + ConfigWithoutMCP), parser tests updated (empty/no-MCP → no_mcp, malformed → unknown, new no_mcp regression)
Integration / package tests — existing discovery/models suites updated and passing
More than 3 test cases (1 happy path + multiple error/edge cases)
Stress / soak test: N/A (no long-running or async code in this phase)
Local gates: make fmt, make vet, make build, make lint (0 issues), make test (race + coverage) all pass.
Manual verification reviewers can run:
- ./bin/agent-scanner mcp-server and install-mcp-server → exit 1 (unknown command)
- ./bin/agent-scanner --help → neither command listed
- scan / inspect / clients / version still work

Verifiability check

Inputs and outputs documented (config types, code constants, alias semantics)
Reviewer can judge correctness from the table-driven tests + type signatures
No new runtime/monitoring surface (pure removal + parsing/model changes)

Risk & rollback

Risk: This is a breaking change — mcp-server / install-mcp-server are gone; any user/script invoking them will get an unknown-command error. The parser now classifies valid-but-no-MCP files as no_mcp instead of unknown (intentional; updated assertions).
Rollback: Single self-contained commit on a feature branch — revert the commit to restore the removed package, deps, and prior parser behavior.

Reviewer guide

Read carefully:
- internal/models/server.go + internal/models/config.go — serverUrl alias precedence (canonical url wins) and the new ConfigWithoutMCP type
- internal/discovery/parser.go — the valid-JSON-vs-unparseable branch that decides no_mcp vs unknown
- internal/models/errors.go + issue.go — X010 / skipped_by_runtime_config mapping
- go.mod / go.sum — confirm only go-sdk + its transitive deps were dropped
Spot-check OK:
- internal/cli/{root,flags}.go — mechanical removal of the two command registrations + flags struct
- The large internal/mcpserver/ deletions and README section removal
- Updated test files (assertions follow directly from the type/parser changes)

🤖 Generated with Claude Code

- Remove the mcp-server and install-mcp-server commands and their package - Drop the now-unused go-sdk dependency and tidy modules - Add the X010 skipped-by-runtime-config code and category mapping - Accept serverUrl as an alias for url when parsing server configs - Introduce a no_mcp config type so empty configs parse without error BREAKING CHANGE: the mcp-server and install-mcp-server commands are removed; run agent-scanner via scan/inspect instead. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR removes the experimental mcp-server / install-mcp-server CLI modes (including the internal/mcpserver/ package and its Go SDK dependency) and introduces Phase-0 “scan foundations” for later phases: a new skip-result code/category mapping, a serverUrl config alias, and a config type representing valid configs with no MCP servers.

Changes:

Removed MCP server mode + installer commands and deleted the internal/mcpserver/ implementation and tests.
Added serverUrl → url alias handling (both in raw config parsing and RemoteServer JSON decoding).
Added ConfigWithoutMCP (ConfigType()=="no_mcp") and updated discovery parsing/tests; added new error category → issue code mapping for skipped_by_runtime_config (X010).

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
README.md	Removes documentation for MCP server mode.
internal/models/server.go	Adds `RemoteServer.UnmarshalJSON` supporting `serverUrl` alias.
internal/models/issue.go	Adds `X010` (`CodeSkippedByRuntimeConfig`).
internal/models/errors.go	Adds `ErrCatSkippedByRuntimeConfig` and maps it to `X010`.
internal/models/errors_test.go	Adds tests for the full category→code map (incl. `X010`).
internal/models/config.go	Adds `ConfigWithoutMCP`; adds `serverUrl` alias in `ServerConfigJSON` and `ToServerConfig`.
internal/models/config_test.go	Adds tests for `serverUrl` alias paths and `ConfigWithoutMCP`.
internal/discovery/parser.go	Changes fallback parsing to return `no_mcp` for valid JSON with no servers.
internal/discovery/parser_test.go	Updates “unknown” test semantics and adds `no_mcp` coverage.
internal/discovery/parser_testdata_test.go	Updates testdata expectations for `no_mcp` vs `unknown`.
internal/cli/root.go	Unregisters removed `mcp-server` / `install-mcp-server` commands.
internal/cli/flags.go	Removes `MCPServerFlags` global state.
internal/cli/mcpserver.go	Deleted MCP server CLI command implementation.
internal/cli/install.go	Deleted installer CLI command implementation.
internal/mcpserver/server.go	Deleted MCP server implementation.
internal/mcpserver/server_test.go	Deleted MCP server tests.
internal/mcpserver/install.go	Deleted config installer implementation.
internal/mcpserver/install_test.go	Deleted installer tests.
go.mod	Removes `modelcontextprotocol/go-sdk` and related indirect deps after deletion.
go.sum	Removes checksums for dropped dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Treat JSON type mismatches in recognized config shapes as unknown format - Restrict the no_mcp fallback to JSON objects, excluding arrays and scalars - Add regression tests for malformed and non-object config inputs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

- Return a non-nil empty map for consistency with other MCPConfig types - Update the test to assert a non-nil empty server set Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.

- Classify configs with an explicit null for a known MCP key as unknown - Keep valid empty objects classified as no_mcp - Add tests for null MCP keys and the empty-object regression Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 19 out of 20 changed files in this pull request and generated no new comments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings May 23, 2026 09:42

Copilot started reviewing on behalf of appleboy May 23, 2026 09:43 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread internal/discovery/parser.go Outdated

appleboy requested a review from Copilot May 23, 2026 09:52

Copilot started reviewing on behalf of appleboy May 23, 2026 09:53 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread internal/models/config.go Outdated

Comment thread internal/models/config_test.go

refactor(models): return empty map from ConfigWithoutMCP.GetServers

b6809fc

- Return a non-nil empty map for consistency with other MCPConfig types - Update the test to assert a non-nil empty server set Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

appleboy requested a review from Copilot May 23, 2026 09:59

Copilot started reviewing on behalf of appleboy May 23, 2026 10:00 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread internal/discovery/parser.go

Comment thread internal/discovery/parser_test.go

appleboy requested a review from Copilot May 23, 2026 10:06

Copilot started reviewing on behalf of appleboy May 23, 2026 10:06 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

docs(claude): drop deleted mcpserver package from structure list

96c71f0

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

appleboy merged commit 79096d5 into main May 23, 2026
12 checks passed

appleboy deleted the feat/phase0-foundations-remove-mcpserver branch May 23, 2026 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli)!: remove mcp-server mode and add scan foundations (Phase 0)#12

feat(cli)!: remove mcp-server mode and add scan foundations (Phase 0)#12
appleboy merged 5 commits into
mainfrom
feat/phase0-foundations-remove-mcpserver

appleboy commented May 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

appleboy commented May 23, 2026

Summary

AI Authorship

Change classification

Plan reference

Verification

Verifiability check

Risk & rollback

Reviewer guide

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants