diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f3eba42..c684e027 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,72 @@ is "names a tag that exists", so it holds on the first commit after the tag too. +- **Four lexer defects in the MCP registration reader, fixed in both + implementations.** (#485 review) Found reviewing the zero-install port; all + four were in the reader #431 shipped, so the port had copied them rather than + introduced them. Two invent a tool name, which is the one outcome a reader of + a *name* cannot afford, and two lose a whole file's surface: + + - A `${…}` holds code, so a brace inside a string, comment, regex or nested + template is not a structural brace. ``const msg = `brace: ${"{"}`;`` left + the substitution open and consumed the rest of the file as one unterminated + template — every registration after that line gone, and a workspace + declaring an MCP dependency reported as "not an agent project" over a brace + in a string. + - A line break ends a JavaScript initializer only when what follows cannot + continue the expression. `static toolName = "safe"` with `+ "_delete"` on + the next line published `safe` at `medium` confidence for a tool the server + registers as `safe_delete`. + - The regex heuristic now resolves the keyword in front of a slash from the + *masked* source. Read from the raw text, a comment between `if` and its + condition hid the keyword, the slash was read as division, and the pattern + was scanned as code — reporting a tool invented out of a regex body, which + is precisely what masking exists to make impossible. + - A backslash before CRLF is one line continuation, not `\r` plus a line + break. The identical file resolved its registration on a Unix checkout and + lost it on a Git-for-Windows one. + + Each is an expected-result case in `tests/mcp_idiom_corpus.py`, so both + readers are pinned to the corrected behaviour rather than to each other's + agreement, and the CRLF sweep now has a continuation case that actually + exercises it. The three vendor servers this input exists for are unaffected — + 61, 114 and 114 tools before and after. + +- **The zero-install detector reads MCP registration sites, so it stops + telling vendor MCP server maintainers to stop.** (#485) `tools/shipgate-detect.py` + is the documented first command run against a repository that has *not* + adopted Shipgate — which is every repository #431 was about. #431 taught the + installed CLI to read a tool's name out of a TypeScript or Go registration + site; the script did not gain it, so the two disagreed on the one question + the script exists to answer: `mongodb-js/mongodb-mcp-server` (61 tools), + `github/github-mcp-server` (110) and `grafana/mcp-grafana` (114) were agent + projects to the CLI and "Stop, not an agent project" to the script. The + masking lexer, the five idioms, the path predicate, the dependency gate and + the export-precedence rule are now all in the script too, stdlib-only. + + Porting a load-bearing matcher means a second implementation of it, which is + this repository's recurring bug class. What makes it affordable is that the + two are not allowed to become *different* implementations: every case either + reader has ever been asked about now lives once in `tests/mcp_idiom_corpus.py` + — every idiom's positive sample, the whole adversarial sweep, the path + predicate's cases and both escape grammars — and both readers are driven + through all of it, compared site by site including each site's byte span. + `samples/mcp_source_only_server` puts the route inside the existing + `samples/` parity sweep, nine constructed workspaces pin the branches around + it (covering export, partial export, wildcard export, no dependency, no + resolved registration, test-only registrations, two registration + directories), and `test_framework_vocabulary_names_every_cli_omission` now + passes with an empty `known_omissions`. + + One defect surfaced while porting and is fixed in both: with no MCP export in + the workspace at all, `_covering_export` returned every resolved name as + "uncovered", and the caller renders a shortfall as *"An MCP tool export is + also present and does not name N of these registrations"*. A server whose + surface exists only as source is the population this input was built for, so + that claim about a file that does not exist was published into the adoption + evidence for every one of them. + + - **No corpus case is graded against `insufficient_evidence` any more, and four `blocked` cells hold one case instead of two.** (#520, #508) A verdict exists to route a change somewhere: `passed` merges, `review_required` hands a human diff --git a/docs/zero-install.md b/docs/zero-install.md index 97fc768b..c6a0147e 100644 --- a/docs/zero-install.md +++ b/docs/zero-install.md @@ -35,21 +35,27 @@ The script's output is a **structural subset** of `agents-shipgate detect --json "python_parse_truncated": false, "next_action": "agents-shipgate init --workspace .", "workspace_signals": {...}, - "script_version": "0.4.0" + "script_version": "0.5.0" } ``` Like the canonical CLI, the script parse-probes each glob-matched MCP/OpenAPI candidate before suggesting it — a filename match is not a guarantee. A Cursor plugin `mcp.json` is an `mcpServers`-style host config, not an MCP tools-array export; suggesting it would make the next `init --write` → `scan` step fail. Rejected candidates appear under `excluded_sources` (`{type, path, reason}`) instead of `suggested_sources`. The probe is **JSON-only** (stdlib has no YAML parser): a `.json` candidate the adapters would reject is excluded here too, while a `.yaml`/`.yml` OpenAPI spec is always kept as a suggestion (never wrongly dropped). The real-world miss this guards against — `mcpServers`-style host configs — is always JSON, so the probe is exact where it matters. +An MCP server whose tool surface exists **only as TypeScript or Go registration sites** — `mongodb-js/mongodb-mcp-server`, `grafana/mcp-grafana`, `github/github-mcp-server` — is detected here too, and suggested as `{"type": "mcp_server_source", "path": "..."}`. That is 100% of the population this script is pointed at: a repository that has not adopted Shipgate. Until v0.5.0 of the script the reader lived only in the installed CLI, so the documented first command answered "Stop, not an agent project" on exactly the repositories the CLI reported as agent projects with 61, 110 and 114 tools. + +Porting it means a second implementation of the load-bearing matcher — the masking lexer and the five registration idioms. It is held to the CLI's answers by a shared conformance corpus rather than by inspection: every positive sample, the whole adversarial sweep, the path predicate and both escape grammars live once in [`tests/mcp_idiom_corpus.py`](https://github.com/ThreeMoonsLab/agents-shipgate/blob/main/tests/mcp_idiom_corpus.py) and are driven through both readers, compared site by site with the byte span of each. Neither reader can change its answer on a case either of them has ever been asked about without the other following. + Like `agents-shipgate detect`, the script silently skips common fixture corpus directories such as `fixtures/`, `_fixtures/`, `__fixtures__/`, `testdata/`, `test_data/`, `test-fixtures/`, `test_fixtures/`, `golden/`, and `goldens/` when they are below the selected workspace. Point `--workspace` directly at a fixture project if you intentionally want to classify that fixture itself. The script and the canonical CLI are pinned to **structural verdict parity** by [`tests/test_zero_install_detector.py`](https://github.com/ThreeMoonsLab/agents-shipgate/blob/main/tests/test_zero_install_detector.py): same `is_agent_project`, same fired frameworks, same suggested sources, same excluded sources, same Codex plugin candidates, and the same manifest-scope verdict (`agent_scope`, `agent_scope_truncated`, `python_parse_truncated`, plus `agent_project_candidates[]`) for every sample in `samples/`. The scope verdict is pinned because an agent that consults the zero-install path must not adopt a scope the CLI would refuse: on a workspace whose agents live in several self-contained projects, both report `agent_scope: "ambiguous"` and neither recommends initializing the root. `agent_scope_truncated` is pinned for the same reason one step down: when the Python parse stopped at its cap in a workspace holding more than one project root, `agent_project_candidates[]` is a lower bound rather than an enumeration, and a caller that reads a truncated list as complete concludes its own project is not an agent project. `python_parse_truncated` is the wider fact both detectors carry: whether the parse stopped at its cap at all, which is what makes every whole-workspace negative — `is_agent_project: false` included — unsafe to act on. Field-by-field byte parity is not pinned and not promised — the script is not a drop-in replacement for the CLI. `agent_name_candidates` is the one field pinned byte for byte, including its ranking and each entry's `rationale[]`. It is not a yes/no signal: it names the agent a generated manifest would declare as the reviewed identity, so a script that ranked differently would point you at a different agent than `init` does. +The `mcp_server_source` detection is the one *framework* pinned past its presence — score, confidence, and evidence lines. The looseness the rest of this contract grants was granted to detections scored from many heuristic signals and described in the script's own words; this one has exactly two scoring inputs and its lines are rendered by a ported function. Both halves matter and neither is visible in a verdict: the declared dependency is what carries the label to `medium`, and `"61 tools"` without `"and 3 more this reader cannot name"` is the over-claim the input exists to avoid. + **When to use this:** you're a coding agent (Claude Code, Codex, Cursor) deciding *whether* to propose Shipgate. The script tells you in one fetch + one Python invocation. The full flow (`init`, `scan`, `apply-patches`) requires the actual install. -**Constraints:** Python 3.12+ on the runner. Evidence/reason strings and absolute framework scores are simplified — the verdict is what's pinned, not the prose. +**Constraints:** Python 3.12+ on the runner. Evidence/reason strings and absolute framework scores are simplified — the verdict is what's pinned, not the prose. `mcp_server_source` is the exception noted above. The workspace inventory does match the canonical CLI's — `git ls-files` when Git can read the workspace, a contained filesystem walk otherwise. That is a correctness requirement, not a speed one: a `.gitignore`d module is invisible to `init`, so a script that walked it anyway could name an agent `init` will never write. Paths escaping the workspace through a symlink are dropped for the same reason. diff --git a/samples/README.md b/samples/README.md index 88c16a8a..d8742eab 100644 --- a/samples/README.md +++ b/samples/README.md @@ -119,6 +119,8 @@ the pointer for the reader, not merely for the test. | [`multi_agent_workspace`](multi_agent_workspace/) | Multiple manifests in one workspace. | | [`baseline_workflow`](baseline_workflow/) | Baseline adoption before strict CI. | | [`large_multi_framework_agent`](large_multi_framework_agent/) | Production-shape retail-ops agent with ~65 unique tools across 6 declared sources, including a reviewed SDK inventory. Exercises the pipeline at scale and pins the CI latency budget. No committed goldens — see the per-sample README. | +| [`mcp_only_server`](mcp_only_server/) | An MCP server that commits its surface as a `tools/list` export. | +| [`mcp_source_only_server`](mcp_source_only_server/) | The same server with no export: its tools exist only as TypeScript registration sites, which is the normal state of a vendor MCP server. Detected identically by the CLI and the zero-install script (#485). | | [`_anti_patterns`](_anti_patterns/) | Intentionally unsafe or invalid examples for tests and docs. | ## Direct scans diff --git a/samples/mcp_source_only_server/README.md b/samples/mcp_source_only_server/README.md new file mode 100644 index 00000000..20e25f2c --- /dev/null +++ b/samples/mcp_source_only_server/README.md @@ -0,0 +1,70 @@ +# Source-only MCP Tool Server + +A minimal MCP server whose tool surface exists **only in its TypeScript +registration sites**. There is no `tools/list` export to read, which is the +normal state of a vendor MCP server: the official MongoDB and Grafana servers +both publish dozens of tools and commit no export at all. + +Contrast with [`mcp_only_server`](../mcp_only_server/), which is the same kind +of server with its surface committed as [`mcp/tools.json`](../mcp_only_server/mcp/tools.json). +Where both exist, the export wins — it is the server's own published contract, +it carries the input schemas, and it is read at `high` confidence against this +route's `medium`. + +## What it pins + +`detect` reports `is_agent_project: true` and suggests +`{"type": "mcp_server_source", "path": "src"}` — and **both** detectors do: +the installed CLI and the zero-install `tools/shipgate-detect.py`, which is the +documented first command run against a repository that has not adopted +Shipgate. Until #485 the script had no such reader, so it answered "Stop" on +exactly the repositories the CLI had just learned to read; this fixture is what +puts that route inside the parity sweep in +[`tests/test_zero_install_detector.py`](../../tests/test_zero_install_detector.py). + +Two tools are named, one registration is not: + +- `support.search_kb` — registered at the call site in + [`src/server.ts`](src/server.ts), named by a string literal. +- `support.drop_ticket_archive` — a tool class in + [`src/tools/dropTicketArchive.ts`](src/tools/dropTicketArchive.ts) with a + `static toolName` field, plus the sibling `description` and `operationType` + literals from the same class body. +- The `server.registerTool(DropTicketArchiveTool.toolName, …)` call in that + same file passes a reference, not a literal. It is reported as + **unenumerated** rather than dropped, which is why the evidence line says + `1 registration(s) name themselves at runtime and are not enumerated`. A + count without that sentence would be an over-claim. + +The doc comment in `dropTicketArchive.ts` contains a registration too. It is +invisible to both readers: comments and string bodies are masked before +anything is matched, so a documented example can never enter the catalog. + +## The whole walk + +The route is only worth suggesting if the step after it can act on it, so the +fixture is checked end to end rather than at `detect` alone: + +```bash +cp -R samples/mcp_source_only_server /tmp/server +./shipgate detect --workspace /tmp/server --json +./shipgate init --workspace /tmp/server --write +./shipgate scan --config /tmp/server/shipgate.yaml +``` + +`init` writes `tool_sources: [{id: mcp_src_src, type: mcp_server_source, path: +src}]`, and `scan` reads both registrations into `report.json`'s `tool_catalog` +at `medium` confidence, each carrying the file and line it was registered at. +The registration that names itself at runtime is in `surface_exclusions` as +`name_not_literal` — accounted for, not dropped, which is what keeps the count +of two honest. + +The terminal then says `Surface: 0 tools` and stops at `insufficient_evidence`, +and that is the right answer rather than a contradiction: an MCP server has no +agent object, so nothing binds the catalog's tools to a reviewed surface yet — +`0/2 catalog tools reachable`. The next step is named +(`shipgate.yaml#tool_sources[].binding`) and a ready-to-review block is written +to `suggested-declarations.yaml`. [`mcp_only_server`](../mcp_only_server/), +whose surface is a committed export, stops in exactly the same place; the +difference between the two fixtures is how the tools were *found*, not what +happens after. diff --git a/samples/mcp_source_only_server/package.json b/samples/mcp_source_only_server/package.json new file mode 100644 index 00000000..6073df44 --- /dev/null +++ b/samples/mcp_source_only_server/package.json @@ -0,0 +1,11 @@ +{ + "name": "@example/support-mcp-server", + "version": "0.1.0", + "private": true, + "description": "A support MCP server whose tool surface exists only in its TypeScript registration sites.", + "type": "module", + "dependencies": { + "@modelcontextprotocol/sdk": "^1.12.0", + "zod": "^3.23.8" + } +} diff --git a/samples/mcp_source_only_server/src/server.ts b/samples/mcp_source_only_server/src/server.ts new file mode 100644 index 00000000..d7fa9e78 --- /dev/null +++ b/samples/mcp_source_only_server/src/server.ts @@ -0,0 +1,24 @@ +import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; +import { z } from "zod"; + +import { searchKnowledgeBase } from "./handlers.js"; +import { DropTicketArchiveTool } from "./tools/dropTicketArchive.js"; + +export function createServer(): McpServer { + const server = new McpServer({ name: "support", version: "0.1.0" }); + + // Registered at the call site. The only thing read out of this file is the + // string literal in first-argument position; the schema below is never + // evaluated, and the description is not taken from here. + server.registerTool( + "support.search_kb", + { + description: "Search support knowledge-base articles by free-text query.", + inputSchema: { query: z.string() }, + }, + searchKnowledgeBase, + ); + + DropTicketArchiveTool.register(server); + return server; +} diff --git a/samples/mcp_source_only_server/src/tools/dropTicketArchive.ts b/samples/mcp_source_only_server/src/tools/dropTicketArchive.ts new file mode 100644 index 00000000..3ba26736 --- /dev/null +++ b/samples/mcp_source_only_server/src/tools/dropTicketArchive.ts @@ -0,0 +1,25 @@ +import type { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; + +type OperationType = "create" | "read" | "update" | "delete"; + +/** + * The class shape the official MongoDB server uses: the tool's identity is a + * static field, and the sibling `description` and `operationType` literals are + * read from the same class body. + * + * The registration written in this comment — `static toolName = "example"` — + * is invisible to the reader. Comments and string bodies are masked before + * anything is matched, so a documented example can never enter the catalog. + */ +export class DropTicketArchiveTool { + public static readonly toolName: string = "support.drop_ticket_archive"; + public static operationType: OperationType = "delete"; + public description = "Delete the archived-ticket collection for a workspace."; + + static register(server: McpServer): void { + // Deliberately *not* a literal: the reader reports this registration as + // unenumerated rather than dropping it, which is what makes the tool count + // in `detect`'s evidence honest about what it could not name. + server.registerTool(DropTicketArchiveTool.toolName, {}, () => undefined); + } +} diff --git a/src/agents_shipgate/cli/discovery/mcp_source.py b/src/agents_shipgate/cli/discovery/mcp_source.py index 677509dd..03995262 100644 --- a/src/agents_shipgate/cli/discovery/mcp_source.py +++ b/src/agents_shipgate/cli/discovery/mcp_source.py @@ -366,8 +366,17 @@ def _covering_export( restatement, and only then is withholding it lossless. """ + candidates = sorted(exported_source_paths) + if not candidates: + # No export at all, which is the *common* case here: a server whose + # tool surface exists only as source is the population this input + # exists for. `names - covered` would be every name, and the caller + # renders a non-empty shortfall as "an MCP tool export is also present + # and does not name them" — a statement about a file that does not + # exist, in the evidence a human reads when deciding whether to adopt. + return None, set() covered: set[str] = set() - for candidate in sorted(exported_source_paths): + for candidate in candidates: exported = _export_tool_names(workspace, candidate) if exported is None: continue diff --git a/src/agents_shipgate/cli/scope_routing.py b/src/agents_shipgate/cli/scope_routing.py index 94a9377c..ba71c321 100644 --- a/src/agents_shipgate/cli/scope_routing.py +++ b/src/agents_shipgate/cli/scope_routing.py @@ -188,7 +188,7 @@ def scope_candidate_actions( **A candidate that already carries a manifest routes to ``doctor``, not to ``init``.** A nested ``shipgate.yaml`` is itself evidence of a project, so adopted directories are candidates too — on this repository's own - ``samples/``, 21 of 22 are — and ``init --write`` there exits 2 on a + ``samples/``, 23 of 25 are — and ``init --write`` there exits 2 on a manifest it will not overwrite while ``expects`` promises a file that already exists. ``doctor`` is the command that answers what is actually outstanding for an adopted project, and it is the same handoff ``detect`` diff --git a/src/agents_shipgate/inputs/mcp_idioms.py b/src/agents_shipgate/inputs/mcp_idioms.py index 6380d5b0..d66c250f 100644 --- a/src/agents_shipgate/inputs/mcp_idioms.py +++ b/src/agents_shipgate/inputs/mcp_idioms.py @@ -443,8 +443,18 @@ def idioms_for_language(language: SourceLanguage) -> tuple[RegistrationIdiom, .. "'": "'", '"': '"', } -#: JavaScript adds a backtick, ``\0`` for NUL, and a line continuation. -_TYPESCRIPT_ESCAPES = {**_SHARED_ESCAPES, "`": "`", "\n": ""} +#: JavaScript adds a backtick and ``\0`` for NUL. Line continuations are not +#: here: a continuation is a backslash followed by a *line terminator +#: sequence*, and CRLF is one such sequence rather than a ``\r`` escape +#: followed by a break, so it needs a rule that can consume two characters. +_TYPESCRIPT_ESCAPES = {**_SHARED_ESCAPES, "`": "`"} + +#: The line terminators a backslash can continue a line across. ``\r`` is here +#: because a CRLF checkout spells the same continuation with two characters, +#: and JavaScript reads both files identically — so a reader that lost the +#: registration on one of them would answer "not an agent project" for a +#: line-ending translation (#485 review). +_TYPESCRIPT_LINE_TERMINATORS = frozenset("\n\r") #: Go adds the bell and has no line continuation and no bare ``\0``. _GO_ESCAPES = {**_SHARED_ESCAPES, "a": "\a"} @@ -560,6 +570,12 @@ def _decode_typescript(body: str) -> str | None: if index + 1 >= length: return None marker = body[index + 1] + if marker in _TYPESCRIPT_LINE_TERMINATORS: + # A LineContinuation contributes nothing to the value. CRLF is one + # terminator sequence: reading it as `\r` plus a stray line break + # both mangles the value and, in the scanner, ends the string. + index += 3 if marker == "\r" and body[index + 2 : index + 3] == "\n" else 2 + continue if marker in _TYPESCRIPT_ESCAPES: out.append(_TYPESCRIPT_ESCAPES[marker]) index += 2 @@ -692,13 +708,22 @@ def _previous_significant(masked: list[str], index: int) -> tuple[str, int]: return (masked[index], index) if index >= 0 else ("", -1) -def _preceding_word(text: str, index: int) -> str: - while index >= 0 and text[index].isspace(): +def _preceding_word(masked: list[str], index: int) -> str: + """The identifier ending at or before ``index``, read from the mask. + + The mask, not the raw text: comments have been overwritten with spaces + there, so `if /*why*/ (ok) /re/` still finds `if`. Reading the raw text + found `/` — the tail of the comment — decided the slash was division, and + scanned the regex body as code, which reported a tool invented out of a + pattern. That is the one outcome masking exists to make impossible. + """ + + while index >= 0 and masked[index].isspace(): index -= 1 end = index + 1 - while index >= 0 and (text[index].isalnum() or text[index] in "_$"): + while index >= 0 and (masked[index].isalnum() or masked[index] in "_$"): index -= 1 - return text[index + 1 : end] + return "".join(masked[index + 1 : end]) def _mask_typescript(text: str) -> MaskedSource: @@ -732,14 +757,14 @@ def _mask_typescript(text: str) -> MaskedSource: if char == "`": index = _consume_template(masker, index) continue - if char == "/" and _opens_regex(masker.out, text, index): + if char == "/" and _opens_regex(masker.out, index): index = _consume_regex(masker, index) continue index += 1 return masker.result() -def _opens_regex(out: list[str], text: str, index: int) -> bool: +def _opens_regex(out: list[str], index: int) -> bool: previous, previous_index = _previous_significant(out, index - 1) if previous == "" or previous in _REGEX_PRECEDING_CHARS: return True @@ -756,9 +781,9 @@ def _opens_regex(out: list[str], text: str, index: int) -> bool: opener = _matching_open(out, previous_index) if opener is None: return False - return _preceding_word(text, opener - 1) in _REGEX_PRECEDING_STATEMENTS + return _preceding_word(out, opener - 1) in _REGEX_PRECEDING_STATEMENTS if previous.isalnum() or previous in "_$": - return _preceding_word(text, index - 1) in _REGEX_PRECEDING_WORDS + return _preceding_word(out, index - 1) in _REGEX_PRECEDING_WORDS return False @@ -777,6 +802,24 @@ def _matching_open(out: list[str], close_index: int) -> int | None: return None +def _past_escape(text: str, index: int, language: SourceLanguage) -> int: + """The index just past the escape whose backslash sits at ``index``. + + Two characters, except for a JavaScript line continuation spelled with + CRLF, which is three: the backslash and one *line terminator sequence*. + Stepping over two of them leaves the ``\n`` behind, and the scanner then + ends the string there — so the identical file lost its registration on a + Git-for-Windows checkout while resolving it on a Unix one. + + Go has no line continuation, and its scanner must keep treating a newline + as the end of an interpreted string, so this is TypeScript's rule only. + """ + + if language == "typescript" and text[index + 1 : index + 3] == "\r\n": + return index + 3 + return index + 2 + + def _consume_quoted( masker: _Masker, start: int, quote: str, *, allow_newline: bool ) -> int: @@ -786,7 +829,7 @@ def _consume_quoted( while index < length: char = text[index] if char == "\\": - index += 2 + index = _past_escape(text, index, masker.language) continue if char == quote: masker.record( @@ -809,41 +852,142 @@ def _consume_template(masker: _Masker, start: int) -> int: """Consume a backtick template literal, tracking ``${…}`` substitutions.""" text = masker.text + length = len(text) + end, substituted = _template_end(text, masker.out, start) + if end is None: + masker.blank(start, length, _STRING_FILL) + masker.anomalies.append("unterminated_string") + return length + body = text[start + 1 : end - 1] + masker.record( + start, end, None if substituted else decode_literal(body, masker.language) + ) + return end + + +def _template_end( + text: str, out: list[str], start: int +) -> tuple[int | None, bool]: + """Where the template literal at ``start`` ends, and whether it substitutes. + + ``None`` when it never closes. The second value says whether the *outer* + template carries a ``${…}``, which is what makes its value non-constant. + + **A `${…}` holds code, so a brace inside a string, a comment, a regex or a + nested template is not a structural brace.** Counting them made + ``const msg = `Literal brace: ${"{"}`;`` leave the substitution open, and + from there the rest of the file was consumed as one unterminated template + — every registration after that line silently gone, and a workspace that + declares an MCP dependency reported as "not an agent project" over a brace + in a string (#485 review). + + Iterative, with one stack entry per open template, because a nested + template is reached through a substitution and recursion on attacker-shaped + input is a crash rather than a wrong answer. + """ + length = len(text) index = start + 1 + # One entry per open template: its `${…}` brace depth, 0 in template text. + depths: list[int] = [0] substituted = False - depth = 0 - while index < length: + while index < length and depths: char = text[index] if char == "\\": - index += 2 - continue - if depth == 0 and char == "$" and index + 1 < length and text[index + 1] == "{": - substituted = True - depth = 1 - index += 2 + index = _past_escape(text, index, "typescript") continue - if depth > 0: - if char == "{": - depth += 1 - elif char == "}": - depth -= 1 + if depths[-1] == 0: + if char == "$" and text[index + 1 : index + 2] == "{": + substituted = substituted or len(depths) == 1 + depths[-1] = 1 + index += 2 + continue + if char == "`": + depths.pop() + index += 1 + continue index += 1 continue + if char in {"'", '"'}: + index = _skip_quoted(text, index) + continue if char == "`": - body = text[start + 1 : index] - masker.record( - start, - index + 1, - None if substituted else decode_literal(body, masker.language), - ) + depths.append(0) + index += 1 + continue + if char == "/" and text[index + 1 : index + 2] == "/": + line_end = text.find("\n", index) + line_end = length if line_end == -1 else line_end + # Blanked as it is walked, not merely stepped over: the regex + # heuristic below reads the mask to find the keyword in front of a + # slash, and a comment still spelled out there hides it. + out[index:line_end] = _COMMENT_FILL * (line_end - index) + index = line_end + continue + if char == "/" and text[index + 1 : index + 2] == "*": + close = text.find("*/", index + 2) + block_end = length if close == -1 else close + 2 + out[index:block_end] = [ + "\n" if character == "\n" else _COMMENT_FILL + for character in text[index:block_end] + ] + index = block_end + continue + if char == "/" and _opens_regex(out, index): + index = _skip_regex(text, index) + continue + if char == "{": + depths[-1] += 1 + elif char == "}": + depths[-1] -= 1 + index += 1 + return (index if not depths else None), substituted + + +def _skip_quoted(text: str, start: int) -> int: + """Index just past a quoted string this reader only needs to walk over.""" + + quote = text[start] + length = len(text) + index = start + 1 + while index < length: + char = text[index] + if char == "\\": + index = _past_escape(text, index, "typescript") + continue + if char == quote: return index + 1 + if char == "\n": + # Unterminated on its line. Resync there rather than swallowing the + # rest of the substitution. + return index index += 1 - masker.blank(start, length, _STRING_FILL) - masker.anomalies.append("unterminated_string") return length +def _skip_regex(text: str, start: int) -> int: + """Index just past a regex literal, or one past the slash if it is not one.""" + + length = len(text) + index = start + 1 + in_class = False + while index < length: + char = text[index] + if char == "\\": + index += 2 + continue + if char == "\n": + return start + 1 + if char == "[": + in_class = True + elif char == "]": + in_class = False + elif char == "/" and not in_class: + return index + 1 + index += 1 + return start + 1 + + def _consume_regex(masker: _Masker, start: int) -> int: text = masker.text length = len(text) @@ -988,6 +1132,13 @@ def _resolve_name(value: str | None, found: bool) -> tuple[str | None, str | Non return value, None +#: Characters that continue an expression rather than beginning a statement. +#: Consulted only *after* a line break, and only for a character the caller's +#: own terminators do not claim: Go ends a struct field with `,` on the next +#: line, and that comma ends the value rather than continuing it. +_EXPRESSION_CONTINUATION = frozenset("+-*/%&|^<>=!?.,([") + + def _literal_is_whole_value( source: MaskedSource, end: int, terminators: str ) -> bool: @@ -1001,13 +1152,30 @@ def _literal_is_whole_value( the end of input, or at a line break (JavaScript inserts the semicolon). """ + masked = source.masked + length = len(masked) index = end - length = len(source.masked) - while index < length and source.masked[index] in " \t\r": + while index < length and masked[index] in " \t\r": + index += 1 + if index >= length: + return True + if masked[index] in terminators: + return True + if masked[index] != "\n": + return False + # A line break ends the statement only when what follows cannot continue + # the expression. `static toolName = "safe"` followed by `+ "_delete"` on + # the next line is one value spelled across two lines, and accepting the + # first literal publishes `safe` for a tool the server registers as + # `safe_delete` — a name nobody serves, at `medium` confidence, which is + # worse than the omission refusing it produces. Comments are already + # spaces in the mask, so skipping whitespace skips them too. + while index < length and masked[index].isspace(): index += 1 if index >= length: return True - return source.masked[index] in terminators or source.masked[index] == "\n" + following = masked[index] + return following in terminators or following not in _EXPRESSION_CONTINUATION def _call_sites( diff --git a/tests/mcp_idiom_corpus.py b/tests/mcp_idiom_corpus.py new file mode 100644 index 00000000..7fd39625 --- /dev/null +++ b/tests/mcp_idiom_corpus.py @@ -0,0 +1,823 @@ +"""The shared conformance corpus for the MCP registration-idiom readers (#485). + +There are two readers. ``agents_shipgate.inputs.mcp_idioms`` is the installed +CLI's; ``tools/shipgate-detect.py`` carries a stdlib-only port of it, because +the zero-install detector is the documented first command run against a +repository that has *not* adopted Shipgate — which is every vendor MCP server — +and a detector that answered "not an agent project" there sent the maintainer +away from the one route the CLI had just gained. + +A second implementation of a load-bearing matcher is the recurring bug class in +this repository. This module is the reason the duplication is affordable: every +case either reader has ever been asked about lives here once, and both are +driven through all of it — + +* :mod:`tests.test_mcp_idioms` checks the package reader against what each + case must yield — the adversarial sweep records its expected names and + omissions inline, and the named tests own the rest; +* :mod:`tests.test_zero_install_detector` checks the two readers against *each + other* on the same inputs, comparing every field of every site, span + included, plus the path predicate and both escape grammars. + +So a case can only change its answer in one reader if it changes in the other +too. Add a case here, not in either test file: a case added to one test is a +case the other reader was never asked. +""" + +from __future__ import annotations + + +class SourceCase: + """One source text both readers are asked about, with no expectation. + + The expectation for these lives in the test that owns the regression; + what the corpus guarantees is that the *input* reaches both readers. + """ + + def __init__(self, case: str, language: str, text: str) -> None: + self.case = case + self.language = language + self.text = text + + +# --- Positive samples, one per idiom ---------------------------------------- + + +class Sample: + """One idiom's canonical registration, and the name it must yield.""" + + def __init__(self, idiom: str, language: str, text: str, name: str) -> None: + self.idiom = idiom + self.language = language + self.text = text + self.name = name + + +POSITIVE_SAMPLES: dict[str, Sample] = { + "ts_static_tool_name": Sample( + "ts_static_tool_name", + "typescript", + 'export class DropDatabaseTool extends MongoDBToolBase {\n' + ' static toolName = "drop-database";\n' + ' public description = "Removes the specified database";\n' + ' static operationType: OperationType = "delete";\n' + "}\n", + "drop-database", + ), + "ts_sdk_register_tool": Sample( + "ts_sdk_register_tool", + "typescript", + 'server.registerTool("search_docs", { inputSchema: shape }, handler);\n', + "search_docs", + ), + "go_must_tool": Sample( + "go_must_tool", + "go", + "var UpdateIncident = mcpgrafana.MustTool(\n" + '\t"update_incident",\n' + '\t"Update an incident",\n' + "\tupdateIncident,\n" + ")\n", + "update_incident", + ), + "go_new_tool": Sample( + "go_new_tool", + "go", + 'tool := mcp.NewTool("list_workspaces", mcp.WithDescription("List them"))\n', + "list_workspaces", + ), + "go_tool_struct": Sample( + "go_tool_struct", + "go", + "return NewTool(\n" + "\tToolsetMetadataIssues,\n" + "\tmcp.Tool{\n" + '\t\tName: "issue_read",\n' + '\t\tDescription: "Get information about an issue",\n' + "\t\tAnnotations: &mcp.ToolAnnotations{\n" + '\t\t\tTitle: "Get issue details",\n' + "\t\t\tReadOnlyHint: true,\n" + "\t\t},\n" + "\t},\n" + "\thandler,\n" + ")\n", + "issue_read", + ), +} + + +# --- Adversarial sweep ------------------------------------------------------ +# +# Each case names the fail-open or fail-closed it prevents. `expected_names` is +# the complete set the reader may report; `expected_unresolved` the complete +# set of omissions it must record. Both are exact: a case that adds an +# unexpected omission is as wrong as one that loses a tool. + +ADVERSARIAL: list[tuple[str, str, str, list[str], list[str]]] = [ + ( + "a line comment is not code", + "typescript", + '// server.registerTool("ghost", {}, h);\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a doc comment example is not a registration", + "typescript", + "/**\n" + " * Define a tool class:\n" + ' * static toolName = "my-custom-tool";\n' + " */\n" + 'class T { static toolName = "real"; }\n', + ["real"], + [], + ), + ( + "a name inside another string is not a registration", + "typescript", + 'const doc = \'static toolName = "ghost";\';\n' + 'class T { static toolName = "real"; }\n', + ["real"], + [], + ), + ( + "a template literal body is not code", + "typescript", + "const doc = `server.registerTool(\"ghost\", {}, h)`;\n" + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a name built from a constant is unenumerated, not absent", + "typescript", + "class T { static toolName = EXPORT_TOOL_NAME; }\n", + [], + ["name_not_literal"], + ), + ( + "a template substitution is not a constant name", + "typescript", + "server.registerTool(`${prefix}_search`, {}, h);\n", + [], + ["name_not_literal"], + ), + ( + "a concatenated name is not the literal it starts with", + "typescript", + 'server.registerTool("search_" + suffix, {}, h);\n', + [], + ["name_not_literal"], + ), + ( + "a concatenated static field is not the literal it starts with", + "typescript", + 'class T { static toolName = "search_" + SUFFIX; }\n', + [], + ["name_not_literal"], + ), + ( + "a one-argument call is a lookup, not a registration", + "typescript", + 'const t = registry.tool("issues");\n', + [], + [], + ), + ( + "a one-argument call with a computed key is still a lookup", + "typescript", + "const t = registry.tool(key);\n", + [], + [], + ), + ( + "a literal that is not shaped like a tool name is not one", + "typescript", + 'panel.tool("Search the web for a query", handler);\n', + [], + ["implausible_tool_name"], + ), + ( + "a regex literal containing a quote must not desync the lexer", + "typescript", + "const quote = /\"/;\n" 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a regex literal containing a comment opener must not blank the file", + "typescript", + "const path = /a\\/\\/b/;\n" 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a URL inside a string is not a line comment", + "typescript", + 'const url = "https://example.test/x";\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "an escaped quote does not end its string", + "typescript", + 'const s = "he said \\"registerTool\\"";\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a method named toolName is not a field", + "typescript", + 'class T { toolName() { return "ghost"; } }\n', + [], + [], + ), + ( + "a wrapper naming its tool one argument later reports one site", + "go", + "return NewTool(meta, mcp.Tool{Name: \"issue_read\"}, handler)\n", + ["issue_read"], + [], + ), + ( + "a wrapper naming nothing reports exactly one omission", + "go", + "return NewTool(meta, mcp.Tool{Name: name}, handler)\n", + [], + ["name_not_literal"], + ), + ( + "a nested annotation Name is not the tool's name", + "go", + 'mcp.Tool{Annotations: &mcp.ToolAnnotations{Name: "annotation"}}\n', + [], + [], + ), + ( + "a slice of tool structs names every element", + "go", + 'tools := []mcp.Tool{{Name: "a", Description: "x"}, {Name: "b"}}\n', + ["a", "b"], + [], + ), + ( + "a Go raw string names its tool", + "go", + "mcpgrafana.MustTool(`raw_name`, desc, handler)\n", + ["raw_name"], + [], + ), + ( + # A Go raw string is not escape-processed: this registers the literal + # name `raw\137name`, which is not a tool-name shape, so the site is an + # omission. Decoding it the way an interpreted string is decoded yields + # `raw_name` — a name the server does not serve, entered into the + # catalog as though it did. + "a Go raw string is not escape-processed", + "go", + "mcpgrafana.MustTool(`raw\\137name`, desc, handler)\n", + [], + ["implausible_tool_name"], + ), + ( + # A `/` inside a character class does not end the regex. Stop tracking + # the class and the pattern ends early, leaving `']/;` as code — an + # apostrophe that opens a string and swallows the line, which is how + # one regex costs the registrations after it. + "a slash inside a regex character class does not end it", + "typescript", + "const sep = /[/\']/;\n" 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a rune literal holding a quote must not desync the lexer", + "go", + "if c == '\"' {\n}\n" 'mcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + "a Go comment is not a registration", + "go", + '// mcpgrafana.MustTool("ghost", desc, handler)\n' + 'mcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + "NewToolResultError is not NewTool", + "go", + 'return utils.NewToolResultError("boom"), nil\n', + [], + [], + ), + ( + # `mcpgrafana.MustTool(` matches because a `.` is not a word character; + # a helper of the repository's own whose name merely *ends* in + # `MustTool` is a different function, and reading it registers a tool + # nobody serves. + "an identifier ending in MustTool is not MustTool", + "go", + 'registerMustTool("ghost", desc, handler)\n', + [], + [], + ), + ( + "an identifier ending in NewTool is not NewTool", + "go", + 'buildNewTool("ghost", desc, handler)\n', + [], + [], + ), + ( + # Same boundary on the struct idiom, and the case the existing + # `ToolDependencies` sample does not reach: that name has text between + # `Tool` and the brace, so it never matched. A type whose name *ends* + # in `Tool` does. + "a type whose name ends in Tool is not mcp.Tool", + "go", + 'handler := RegistryTool{Name: "ghost", Description: "x"}\n', + [], + [], + ), + ( + "a type whose name merely ends in Tool is not a tool struct", + "go", + 'deps := ToolDependencies{Name: "not_a_tool"}\n', + [], + [], + ), + ( + "a Go octal escape decodes with Go's grammar, not JavaScript's", + "go", + 'mcpgrafana.MustTool("delete\\137all", desc, handler)\n', + ["delete_all"], + [], + ), + ( + "a Go hex and unicode escape decode exactly", + "go", + 'mcpgrafana.MustTool("\\x61\\u0062c", desc, handler)\n', + ["abc"], + [], + ), + ( + "an escape Go does not define is refused, not guessed", + "go", + 'mcpgrafana.MustTool("delete\\qall", desc, handler)\n', + [], + ["name_not_literal"], + ), + ( + "a truncated Go octal escape is refused", + "go", + 'mcpgrafana.MustTool("delete\\13", desc, handler)\n', + [], + ["name_not_literal"], + ), + ( + "a TypeScript hex escape decodes, and octal is refused", + "typescript", + 'server.registerTool("\\x61bc", {}, h);\n' + 'server.registerTool("de\\137f", {}, h);\n', + ["abc"], + ["name_not_literal"], + ), + ( + "a regex beginning an if body cannot register a tool", + "typescript", + 'if (ok) /\\.registerTool("fake", handler)/.test(value);\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a regex beginning a while body cannot register a tool", + "typescript", + 'while (ok) /\\.registerTool("fake", h)/.test(v);\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "division after a call is still division", + "typescript", + "const ratio = total(a) / scale(b) / 2;\n" + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + # Semicolon-less TypeScript, where the statement ends at the line + # break. It is the only shape in which `_literal_is_whole_value` ever + # reaches the character *after* the literal on a CRLF checkout, so + # without it the reader's `\r` handling is untested and a Windows + # checkout silently loses the tool. + "a static field with no semicolon is still the whole value", + "typescript", + 'class T {\n static toolName = "loose_no_semicolon"\n}\n', + ["loose_no_semicolon"], + [], + ), + ( + # A `${…}` holds code, so a brace inside a string is not a structural + # brace. Counting it left the substitution open and consumed the rest + # of the file as one unterminated template: every registration after + # this line silently gone, and a workspace declaring an MCP dependency + # reported as "not an agent project" over a brace in a string. + "a brace inside a template substitution is not a structural brace", + "typescript", + 'const msg = `Literal brace: ${"{"}`;\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + # The brace has to be an *opening* one for these four to discriminate. + # A `}` inside a comment or a regex closes the substitution early and + # the reader then lands on the same closing backtick anyway, so the + # first drafts of these cases passed with the branch deleted. + "a template nested in a substitution closes with its own backtick", + "typescript", + 'const m = `${`}`}`;\n' 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a regex inside a substitution can hold an unbalanced brace", + "typescript", + 'const m = `${s.replace(/[{]/g, "")}`;\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a block comment inside a substitution can hold an unbalanced brace", + "typescript", + 'const m = `${x /* { */}`;\n' 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a line comment inside a substitution can hold an unbalanced brace", + "typescript", + "const m = `${\n x // {\n}`;\n" 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + # A line break ends the statement only when what follows cannot + # continue the expression. Accepting the first literal published + # `safe` for a tool the server registers as `safe_delete` — a name + # nobody serves, at `medium` confidence. + "a concatenation continued on the next line is not the first literal", + "typescript", + "class T {\n" + ' static toolName = "safe"\n' + ' + "_delete";\n' + "}\n", + [], + ["name_not_literal"], + ), + ( + "the same continuation in a Go struct field is refused too", + "go", + "mcp.Tool{\n" '\tName: "issue_"\n' "\t\t+ verb,\n" "}\n", + [], + ["name_not_literal"], + ), + ( + # The regex heuristic resolves the keyword in front of the slash from + # the *masked* source. Read from the raw text, a comment between `if` + # and its condition hid the keyword, the slash was read as division, + # and the pattern was scanned as code — a tool invented out of a regex + # body, which is the one outcome masking exists to make impossible. + "a comment between if and its condition does not hide the keyword", + "typescript", + 'if /*comment*/ (ok) /\\.registerTool("ghost", {}, h)/.test(x);\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + "a comment after typeof does not hide the keyword either", + "typescript", + 'const t = typeof /*c*/ /\\.registerTool("ghost", {}, h)/;\n' + 'server.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + # A backslash before a line terminator is a continuation, and the CRLF + # sweep only exercises it through this case: the same file resolved + # `my_tool` with LF endings and lost the whole registration with CRLF, + # which JavaScript reads identically. + "a line continuation inside a name is one escape", + "typescript", + 'server.registerTool("my\\\n_tool", {}, h);\n', + ["my_tool"], + [], + ), + ( + "a concatenated Go struct name is not the literal it starts with", + "go", + 'mcp.Tool{Name: "issue_" + verb, Description: "x"}\n', + [], + ["name_not_literal"], + ), + # --- Grammar sweep ----------------------------------------------------- + # + # Constructs neither reader had ever been shown, enumerated from the + # lexical grammar rather than from a defect. Maintainer review of #485 + # found four lexer defects that six rounds of *code* perturbation had + # missed, for the reason those rounds could not reach: mutating a branch + # only re-asks the questions the corpus already holds, so it measures the + # corpus rather than extending it. Asking "what construct has this reader + # never been shown?" is the exercise that finds the next one. + # + # All of these passed on the first run. They are recorded so the next + # change to the masker has to keep them passing. + ( + 'a regex holding a backtick does not open a template', + 'typescript', + 'const r = /`/;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a regex holding a template opener does not open one', + 'typescript', + 'const r = /`${/;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a string holding a backtick does not open a template', + 'typescript', + 'const s = "`";\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a template holding a line-comment opener is not a comment', + 'typescript', + 'const t = `//`;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a template holding a block-comment opener is not a comment', + 'typescript', + 'const t = `/*`;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a comment holding a backtick does not open a template', + 'typescript', + '// `\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a comment holding an apostrophe does not open a string', + 'typescript', + '// don\'t\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a block comment holding a quote does not open a string', + 'typescript', + '/* " */\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a dollar not followed by a brace is template text', + 'typescript', + 'const t = `price: $5`;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'an escaped backtick does not close its template', + 'typescript', + 'const t = `a\\`b`;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a regex beginning a statement after a block cannot register', + 'typescript', + 'if (a) {} /\\.registerTool("ghost", {}, h)/.test(b);\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'division after an index is still division', + 'typescript', + 'const q = a[0] / 2 / b;\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a private static field is not the tool name field', + 'typescript', + 'class T { static #toolName = "ghost"; }\nserver.registerTool("real", {}, h);\n', + ["real"], + [], + ), + ( + 'a Go rune holding a backslash must not desync the lexer', + 'go', + 'if c == \'\\\\\' {\n}\nmcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + 'a Go rune holding an escaped quote must not desync the lexer', + 'go', + 'if c == \'\\\'\' {\n}\nmcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + 'a Go raw string holding a quote is not a string boundary', + 'go', + 'var d = `he said "x"`\nmcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + 'a Go raw string holding a comment opener is not a comment', + 'go', + 'var d = `/*`\nmcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), + ( + 'a Go interpreted string holding a backtick is not a raw string', + 'go', + 'var d = "`"\nmcpgrafana.MustTool("real", desc, handler)\n', + ["real"], + [], + ), +] + +# --- Paths the reader is and is not allowed to open ------------------------- +# +# The path predicate decides which files are the surface at all, so a +# disagreement here is a tool one reader can see and the other cannot — before +# any masking happens. + +SCANNABLE_PATHS: list[tuple[str, bool]] = [ + ("pkg/github/issues.go", True), + ("src/tools/aggregate.ts", True), + ("server.mjs", True), + ("pkg/github/issues_test.go", False), + ("src/tools/aggregate.test.ts", False), + ("src/tools/aggregate.spec.ts", False), + ("tests/helpers/fake.ts", False), + ("__tests__/fake.ts", False), + ("node_modules/sdk/index.ts", False), + ("vendor/other/tools.go", False), + ("testdata/sample.go", False), + ("README.md", False), +] + + +# --- Escape grammars -------------------------------------------------------- +# +# One decoder shared between two grammars is a silent mistranslation rather +# than a parse error: Go writes an octal escape as three digits, so +# `MustTool("delete\137all", …)` registers `delete_all` and a +# JavaScript-shaped decoder produced `delete137all` — the real action absent +# and an id nobody serves in its place. Anything either grammar does not +# define is refused, because a refusal becomes a recorded omission and a guess +# becomes a wrong tool name. + +ESCAPE_CASES: list[tuple[str, str, str | None]] = [ + (r"delete\137all", "go", "delete_all"), + # The same bytes in TypeScript are a legacy octal whose meaning depends on + # a strictness mode neither reader tracks, so both refuse. + (r"delete\137all", "typescript", None), + (r"\x41\u0042", "go", "AB"), + (r"\x41\u0042", "typescript", "AB"), + (r"\u{1F600}", "typescript", "\U0001F600"), + (r"a\qb", "go", None), + (r"a\13b", "go", None), + (r"a\400b", "go", None), + (r"a\x4", "go", None), + (r"a\u12", "go", None), + (r"a\x4", "typescript", None), + (r"a\u{}", "typescript", None), + (r"a\1b", "typescript", None), + # Refused by a guard rather than by falling through: drop the hex check and + # `int(digits, 16)` raises out of the scan instead of recording an + # omission, and the caller loses the whole file rather than one name. + (r"a\u{zz}", "typescript", None), + (r"a\u{110000}", "typescript", None), + (r"a\U00110000", "go", None), +] + + +# --- Masking failures ------------------------------------------------------- +# +# Past an unterminated string or block comment neither reader can tell code +# from content, so reporting no sites would be indistinguishable from a file +# that registers nothing — the exact ambiguity this input exists to remove. + +MASKING_FAILURES: dict[str, SourceCase] = { + "unterminated_block_comment": SourceCase( + "unterminated_block_comment", + "typescript", + '/* open\nserver.registerTool("real", {}, h);\n', + ), + "unterminated_string_resyncs_at_the_line": SourceCase( + "unterminated_string_resyncs_at_the_line", + "typescript", + 'const broken = "oops;\n' 'server.registerTool("real", {}, h);\n', + ), + "unterminated_go_raw_string": SourceCase( + "unterminated_go_raw_string", + "go", + 'var doc = `open\nmcpgrafana.MustTool("real", desc, handler)\n', + ), +} + + +# --- Regressions the reader carries scars from ------------------------------ +# +# Each of these was a real defect. They are inputs, not assertions: the test +# that owns each one states what it must yield, and the corpus guarantees the +# other reader is asked the same question. + +REGRESSIONS: dict[str, SourceCase] = { + "static_field_outside_a_class": SourceCase( + "static_field_outside_a_class", + "typescript", + 'static toolName = "loose";\n', + ), + "modifier_between_static_and_the_field": SourceCase( + "modifier_between_static_and_the_field", + "typescript", + 'class T { public static readonly toolName: string = "get_response"; }', + ), + "class_that_registers_elsewhere": SourceCase( + "class_that_registers_elsewhere", + "typescript", + "export class T extends Base {\n" + " static toolName = DYNAMIC_NAME;\n" + ' register(s) { s.registerTool("inner", {}, h); }\n' + "}\n", + ), + "attribute_assignment_is_not_the_description": SourceCase( + "attribute_assignment_is_not_the_description", + "typescript", + "class T {\n" + " constructor() {\n" + ' this.description = "scratch label";\n' + ' this.operationType = "delete";\n' + " }\n" + ' static toolName = "t";\n' + ' public description = "Runs an aggregation";\n' + "}\n", + ), + "masking_preserves_offsets": SourceCase( + "masking_preserves_offsets", + "typescript", + '// comment\n// comment\nclass T { static toolName = "x"; }\n', + ), + "no_registration_token": SourceCase( + "no_registration_token", + "go", + "package main\n\nfunc main() {}\n", + ), +} + + +# --- Everything both readers are driven through ----------------------------- + +SOURCE_CASES: tuple[SourceCase, ...] = ( + *( + SourceCase(f"positive:{idiom}", sample.language, sample.text) + for idiom, sample in sorted(POSITIVE_SAMPLES.items()) + ), + *( + SourceCase(f"adversarial:{case}", language, text) + for case, language, text, _names, _unresolved in ADVERSARIAL + ), + *( + SourceCase(f"masking:{name}", entry.language, entry.text) + for name, entry in sorted(MASKING_FAILURES.items()) + ), + *( + SourceCase(f"regression:{name}", entry.language, entry.text) + for name, entry in sorted(REGRESSIONS.items()) + ), +) diff --git a/tests/test_mcp_idioms.py b/tests/test_mcp_idioms.py index d0a074a7..6b80599b 100644 --- a/tests/test_mcp_idioms.py +++ b/tests/test_mcp_idioms.py @@ -30,6 +30,14 @@ mask_source, scan_source, ) +from tests.mcp_idiom_corpus import ( + ADVERSARIAL, + ESCAPE_CASES, + MASKING_FAILURES, + POSITIVE_SAMPLES, + REGRESSIONS, + SCANNABLE_PATHS, +) def _names(text: str, language: str = "typescript") -> list[str]: @@ -68,7 +76,7 @@ def test_the_prefilter_cannot_hide_an_idiom_this_reader_matches(): by ``scan`` and never by ``detect``. """ - for sample in _POSITIVE_SAMPLES.values(): + for sample in POSITIVE_SAMPLES.values(): assert PREFILTER_TOKEN in sample.text.lower(), sample.idiom @@ -81,21 +89,7 @@ def test_jsx_is_out_of_scope_so_prose_never_opens_a_string(): @pytest.mark.parametrize( - ("path", "scannable"), - [ - ("pkg/github/issues.go", True), - ("src/tools/aggregate.ts", True), - ("server.mjs", True), - ("pkg/github/issues_test.go", False), - ("src/tools/aggregate.test.ts", False), - ("src/tools/aggregate.spec.ts", False), - ("tests/helpers/fake.ts", False), - ("__tests__/fake.ts", False), - ("node_modules/sdk/index.ts", False), - ("vendor/other/tools.go", False), - ("testdata/sample.go", False), - ("README.md", False), - ], + ("path", "scannable"), SCANNABLE_PATHS, ids=[case[0] for case in SCANNABLE_PATHS] ) def test_scannable_paths(path: str, scannable: bool): assert is_scannable_path(path) is scannable @@ -104,74 +98,13 @@ def test_scannable_paths(path: str, scannable: bool): # --- Positive samples, one per idiom ---------------------------------------- -class _Sample: - def __init__(self, idiom: str, language: str, text: str, name: str) -> None: - self.idiom = idiom - self.language = language - self.text = text - self.name = name - - -_POSITIVE_SAMPLES: dict[str, _Sample] = { - "ts_static_tool_name": _Sample( - "ts_static_tool_name", - "typescript", - 'export class DropDatabaseTool extends MongoDBToolBase {\n' - ' static toolName = "drop-database";\n' - ' public description = "Removes the specified database";\n' - ' static operationType: OperationType = "delete";\n' - "}\n", - "drop-database", - ), - "ts_sdk_register_tool": _Sample( - "ts_sdk_register_tool", - "typescript", - 'server.registerTool("search_docs", { inputSchema: shape }, handler);\n', - "search_docs", - ), - "go_must_tool": _Sample( - "go_must_tool", - "go", - "var UpdateIncident = mcpgrafana.MustTool(\n" - '\t"update_incident",\n' - '\t"Update an incident",\n' - "\tupdateIncident,\n" - ")\n", - "update_incident", - ), - "go_new_tool": _Sample( - "go_new_tool", - "go", - 'tool := mcp.NewTool("list_workspaces", mcp.WithDescription("List them"))\n', - "list_workspaces", - ), - "go_tool_struct": _Sample( - "go_tool_struct", - "go", - "return NewTool(\n" - "\tToolsetMetadataIssues,\n" - "\tmcp.Tool{\n" - '\t\tName: "issue_read",\n' - '\t\tDescription: "Get information about an issue",\n' - "\t\tAnnotations: &mcp.ToolAnnotations{\n" - '\t\t\tTitle: "Get issue details",\n' - "\t\t\tReadOnlyHint: true,\n" - "\t\t},\n" - "\t},\n" - "\thandler,\n" - ")\n", - "issue_read", - ), -} - - def test_every_idiom_has_a_positive_sample(): - assert set(_POSITIVE_SAMPLES) == set(IDIOMS_BY_ID) + assert set(POSITIVE_SAMPLES) == set(IDIOMS_BY_ID) -@pytest.mark.parametrize("idiom_id", sorted(_POSITIVE_SAMPLES)) +@pytest.mark.parametrize("idiom_id", sorted(POSITIVE_SAMPLES)) def test_positive_sample_resolves_exactly_its_own_tool(idiom_id: str): - sample = _POSITIVE_SAMPLES[idiom_id] + sample = POSITIVE_SAMPLES[idiom_id] result = scan_source(sample.text, sample.language) resolved = [site for site in result.sites if site.name is not None] assert [site.name for site in resolved] == [sample.name] @@ -179,7 +112,7 @@ def test_positive_sample_resolves_exactly_its_own_tool(idiom_id: str): assert result.anomalies == () -@pytest.mark.parametrize("idiom_id", sorted(_POSITIVE_SAMPLES)) +@pytest.mark.parametrize("idiom_id", sorted(POSITIVE_SAMPLES)) def test_published_diff_tokens_appear_in_the_sample_they_route(idiom_id: str): """The trigger catalog routes on these tokens, so they must be real. @@ -187,20 +120,20 @@ def test_published_diff_tokens_appear_in_the_sample_they_route(idiom_id: str): would say so — the rule would read as coverage it does not have. """ - sample = _POSITIVE_SAMPLES[idiom_id] + sample = POSITIVE_SAMPLES[idiom_id] for token in IDIOMS_BY_ID[idiom_id].diff_tokens: assert token in sample.text, (idiom_id, token) def test_ts_static_field_reads_the_sibling_operation_class_and_description(): - sample = _POSITIVE_SAMPLES["ts_static_tool_name"] + sample = POSITIVE_SAMPLES["ts_static_tool_name"] site = scan_source(sample.text, "typescript").sites[0] assert site.operation_type == "delete" assert site.description == "Removes the specified database" def test_a_go_struct_description_is_read_only_from_its_own_level(): - sample = _POSITIVE_SAMPLES["go_tool_struct"] + sample = POSITIVE_SAMPLES["go_tool_struct"] site = scan_source(sample.text, "go").sites[0] assert site.description == "Get information about an issue" @@ -208,7 +141,8 @@ def test_a_go_struct_description_is_read_only_from_its_own_level(): def test_a_static_field_outside_a_class_still_names_its_tool(): """The enclosing block supplies siblings; its absence is not a failure.""" - sites = scan_source('static toolName = "loose";\n', "typescript").sites + case = REGRESSIONS["static_field_outside_a_class"] + sites = scan_source(case.text, case.language).sites assert [site.name for site in sites] == ["loose"] assert sites[0].operation_type is None @@ -217,277 +151,15 @@ def test_a_modifier_between_static_and_the_field_is_read(): """``public static readonly toolName: string = "…"`` is MongoDB's spelling in four of its packages, and it is not ``static toolName``.""" - text = 'class T { public static readonly toolName: string = "get_response"; }' - assert _names(text) == ["get_response"] - - -# --- Adversarial sweep ------------------------------------------------------ -# -# Each case names the fail-open or fail-closed it prevents. `expected_names` is -# the complete set the reader may report; `expected_unresolved` the complete -# set of omissions it must record. Both are exact: a case that adds an -# unexpected omission is as wrong as one that loses a tool. - -_ADVERSARIAL: list[tuple[str, str, str, list[str], list[str]]] = [ - ( - "a line comment is not code", - "typescript", - '// server.registerTool("ghost", {}, h);\n' - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a doc comment example is not a registration", - "typescript", - "/**\n" - " * Define a tool class:\n" - ' * static toolName = "my-custom-tool";\n' - " */\n" - 'class T { static toolName = "real"; }\n', - ["real"], - [], - ), - ( - "a name inside another string is not a registration", - "typescript", - 'const doc = \'static toolName = "ghost";\';\n' - 'class T { static toolName = "real"; }\n', - ["real"], - [], - ), - ( - "a template literal body is not code", - "typescript", - "const doc = `server.registerTool(\"ghost\", {}, h)`;\n" - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a name built from a constant is unenumerated, not absent", - "typescript", - "class T { static toolName = EXPORT_TOOL_NAME; }\n", - [], - ["name_not_literal"], - ), - ( - "a template substitution is not a constant name", - "typescript", - "server.registerTool(`${prefix}_search`, {}, h);\n", - [], - ["name_not_literal"], - ), - ( - "a concatenated name is not the literal it starts with", - "typescript", - 'server.registerTool("search_" + suffix, {}, h);\n', - [], - ["name_not_literal"], - ), - ( - "a concatenated static field is not the literal it starts with", - "typescript", - 'class T { static toolName = "search_" + SUFFIX; }\n', - [], - ["name_not_literal"], - ), - ( - "a one-argument call is a lookup, not a registration", - "typescript", - 'const t = registry.tool("issues");\n', - [], - [], - ), - ( - "a one-argument call with a computed key is still a lookup", - "typescript", - "const t = registry.tool(key);\n", - [], - [], - ), - ( - "a literal that is not shaped like a tool name is not one", - "typescript", - 'panel.tool("Search the web for a query", handler);\n', - [], - ["implausible_tool_name"], - ), - ( - "a regex literal containing a quote must not desync the lexer", - "typescript", - "const quote = /\"/;\n" 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a regex literal containing a comment opener must not blank the file", - "typescript", - "const path = /a\\/\\/b/;\n" 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a URL inside a string is not a line comment", - "typescript", - 'const url = "https://example.test/x";\n' - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "an escaped quote does not end its string", - "typescript", - 'const s = "he said \\"registerTool\\"";\n' - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a method named toolName is not a field", - "typescript", - 'class T { toolName() { return "ghost"; } }\n', - [], - [], - ), - ( - "a wrapper naming its tool one argument later reports one site", - "go", - "return NewTool(meta, mcp.Tool{Name: \"issue_read\"}, handler)\n", - ["issue_read"], - [], - ), - ( - "a wrapper naming nothing reports exactly one omission", - "go", - "return NewTool(meta, mcp.Tool{Name: name}, handler)\n", - [], - ["name_not_literal"], - ), - ( - "a nested annotation Name is not the tool's name", - "go", - 'mcp.Tool{Annotations: &mcp.ToolAnnotations{Name: "annotation"}}\n', - [], - [], - ), - ( - "a slice of tool structs names every element", - "go", - 'tools := []mcp.Tool{{Name: "a", Description: "x"}, {Name: "b"}}\n', - ["a", "b"], - [], - ), - ( - "a Go raw string names its tool", - "go", - "mcpgrafana.MustTool(`raw_name`, desc, handler)\n", - ["raw_name"], - [], - ), - ( - "a rune literal holding a quote must not desync the lexer", - "go", - "if c == '\"' {\n}\n" 'mcpgrafana.MustTool("real", desc, handler)\n', - ["real"], - [], - ), - ( - "a Go comment is not a registration", - "go", - '// mcpgrafana.MustTool("ghost", desc, handler)\n' - 'mcpgrafana.MustTool("real", desc, handler)\n', - ["real"], - [], - ), - ( - "NewToolResultError is not NewTool", - "go", - 'return utils.NewToolResultError("boom"), nil\n', - [], - [], - ), - ( - "a type whose name merely ends in Tool is not a tool struct", - "go", - 'deps := ToolDependencies{Name: "not_a_tool"}\n', - [], - [], - ), - ( - "a Go octal escape decodes with Go's grammar, not JavaScript's", - "go", - 'mcpgrafana.MustTool("delete\\137all", desc, handler)\n', - ["delete_all"], - [], - ), - ( - "a Go hex and unicode escape decode exactly", - "go", - 'mcpgrafana.MustTool("\\x61\\u0062c", desc, handler)\n', - ["abc"], - [], - ), - ( - "an escape Go does not define is refused, not guessed", - "go", - 'mcpgrafana.MustTool("delete\\qall", desc, handler)\n', - [], - ["name_not_literal"], - ), - ( - "a truncated Go octal escape is refused", - "go", - 'mcpgrafana.MustTool("delete\\13", desc, handler)\n', - [], - ["name_not_literal"], - ), - ( - "a TypeScript hex escape decodes, and octal is refused", - "typescript", - 'server.registerTool("\\x61bc", {}, h);\n' - 'server.registerTool("de\\137f", {}, h);\n', - ["abc"], - ["name_not_literal"], - ), - ( - "a regex beginning an if body cannot register a tool", - "typescript", - 'if (ok) /\\.registerTool("fake", handler)/.test(value);\n' - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a regex beginning a while body cannot register a tool", - "typescript", - 'while (ok) /\\.registerTool("fake", h)/.test(v);\n' - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "division after a call is still division", - "typescript", - "const ratio = total(a) / scale(b) / 2;\n" - 'server.registerTool("real", {}, h);\n', - ["real"], - [], - ), - ( - "a concatenated Go struct name is not the literal it starts with", - "go", - 'mcp.Tool{Name: "issue_" + verb, Description: "x"}\n', - [], - ["name_not_literal"], - ), -] + assert _names(REGRESSIONS["modifier_between_static_and_the_field"].text) == [ + "get_response" + ] @pytest.mark.parametrize( ("case", "language", "text", "expected_names", "expected_unresolved"), - _ADVERSARIAL, - ids=[case[0] for case in _ADVERSARIAL], + ADVERSARIAL, + ids=[case[0] for case in ADVERSARIAL], ) def test_adversarial_constructs( case: str, @@ -523,13 +195,8 @@ def test_a_class_that_registers_elsewhere_keeps_its_own_omission(): miss this input exists to end. """ - text = ( - "export class T extends Base {\n" - " static toolName = DYNAMIC_NAME;\n" - ' register(s) { s.registerTool("inner", {}, h); }\n' - "}\n" - ) - result = scan_source(text, "typescript") + case = REGRESSIONS["class_that_registers_elsewhere"] + result = scan_source(case.text, case.language) assert sorted(site.name for site in result.sites if site.name) == ["inner"] assert [ @@ -546,57 +213,38 @@ def test_an_attribute_assignment_is_not_the_description_field(): report and into the declaration questionnaire a reviewer answers from. """ - text = ( - "class T {\n" - ' constructor() { this.description = "scratch label"; }\n' - ' static toolName = "t";\n' - ' public description = "Runs an aggregation";\n' - "}\n" - ) - site = scan_source(text, "typescript").sites[0] + case = REGRESSIONS["attribute_assignment_is_not_the_description"] + site = scan_source(case.text, case.language).sites[0] assert site.name == "t" assert site.description == "Runs an aggregation" + # `operationType` never had the defect, because its pattern requires a + # leading `static` — and that is a property nothing checked. The class + # declares no static `operationType`, so a scratch assignment in the + # constructor must leave it unset rather than infer a `delete` risk tag + # from it. + assert site.operation_type is None -def test_each_language_decodes_with_its_own_grammar(): +@pytest.mark.parametrize( + ("body", "language", "expected"), + ESCAPE_CASES, + ids=[f"{language}:{body}" for body, language, _expected in ESCAPE_CASES], +) +def test_each_language_decodes_with_its_own_grammar( + body: str, language: str, expected: str | None +): """One decoder shared between two grammars is a silent mistranslation. Go writes an octal escape as three digits and JavaScript does not, so a JavaScript-shaped decoder turned `delete\\137all` — the name Go registers as `delete_all` — into `delete137all`: the real action missing from the - catalog and an action id nobody serves standing in for it. + catalog and an action id nobody serves standing in for it. And what a + grammar does not define is refused rather than guessed, because a refusal + becomes a recorded omission and a guess becomes a wrong tool name. """ - assert decode_literal(r"delete\137all", "go") == "delete_all" - # The same bytes in TypeScript are a legacy octal whose meaning depends on - # a strictness mode this reader does not track, so it refuses. - assert decode_literal(r"delete\137all", "typescript") is None - - assert decode_literal(r"\x41\u0042", "go") == "AB" - assert decode_literal(r"\x41\u0042", "typescript") == "AB" - assert decode_literal(r"\u{1F600}", "typescript") == "\U0001F600" - - -@pytest.mark.parametrize( - ("body", "language"), - [ - (r"a\qb", "go"), - (r"a\13b", "go"), - (r"a\400b", "go"), - (r"a\x4", "go"), - (r"a\u12", "go"), - (r"a\x4", "typescript"), - (r"a\u{}", "typescript"), - (r"a\1b", "typescript"), - ], -) -def test_an_escape_that_cannot_be_decoded_exactly_is_refused( - body: str, language: str -): - """A refusal becomes an omission; a guess becomes a wrong tool name.""" - - assert decode_literal(body, language) is None + assert decode_literal(body, language) == expected # --- Masking failures ------------------------------------------------------- @@ -609,24 +257,22 @@ def test_an_unterminated_block_comment_is_an_anomaly_not_silence(): nothing — the exact ambiguity this input exists to remove. """ - result = scan_source('/* open\nserver.registerTool("real", {}, h);\n', "typescript") + case = MASKING_FAILURES["unterminated_block_comment"] + result = scan_source(case.text, case.language) assert result.sites == () assert result.anomalies == ("unterminated_block_comment",) def test_an_unterminated_string_is_an_anomaly_and_resyncs_at_the_line(): - result = scan_source( - 'const broken = "oops;\n' 'server.registerTool("real", {}, h);\n', - "typescript", - ) + case = MASKING_FAILURES["unterminated_string_resyncs_at_the_line"] + result = scan_source(case.text, case.language) assert "unterminated_string" in result.anomalies assert [site.name for site in result.sites] == ["real"] def test_an_unterminated_go_raw_string_is_an_anomaly(): - result = scan_source( - 'var doc = `open\nmcpgrafana.MustTool("real", desc, handler)\n', "go" - ) + case = MASKING_FAILURES["unterminated_go_raw_string"] + result = scan_source(case.text, case.language) assert result.anomalies == ("unterminated_string",) # And the registration past it is not reported, because past the unclosed # raw string this reader cannot tell code from content. @@ -634,10 +280,10 @@ def test_an_unterminated_go_raw_string_is_an_anomaly(): def test_masking_preserves_offsets_so_line_numbers_are_the_file_s(): - text = "// comment\n// comment\nclass T { static toolName = \"x\"; }\n" - masked = mask_source(text, "typescript") - assert len(masked.masked) == len(text) - assert scan_source(text, "typescript").sites[0].line == 3 + case = REGRESSIONS["masking_preserves_offsets"] + masked = mask_source(case.text, case.language) + assert len(masked.masked) == len(case.text) + assert scan_source(case.text, case.language).sites[0].line == 3 def test_a_file_without_the_prefilter_token_is_answered_without_masking(monkeypatch): @@ -656,14 +302,15 @@ def _fail(*args: object, **kwargs: object) -> None: raise AssertionError("a file with no registration token was masked") monkeypatch.setattr(module, "mask_source", _fail) - assert scan_source("package main\n\nfunc main() {}\n", "go") == SourceScanResult() + case = REGRESSIONS["no_registration_token"] + assert scan_source(case.text, case.language) == SourceScanResult() # And the shortcut is only sound because it can never hide a real # registration: every idiom's own sample carries the token, which # `test_the_prefilter_cannot_hide_an_idiom_this_reader_matches` pins. monkeypatch.undo() assert scan_source( - _POSITIVE_SAMPLES["go_must_tool"].text, "go" + POSITIVE_SAMPLES["go_must_tool"].text, "go" ).sites diff --git a/tests/test_mcp_server_source.py b/tests/test_mcp_server_source.py index 151b9898..d5ddc4f7 100644 --- a/tests/test_mcp_server_source.py +++ b/tests/test_mcp_server_source.py @@ -561,6 +561,25 @@ def test_an_export_for_one_server_does_not_erase_another_s_registrations(tmp_pat assert any("beta_write" in line for line in found.evidence) +def test_no_export_is_not_a_partial_export(tmp_path): + """A shortfall is only a shortfall against an export that exists. + + ``_covering_export`` returned "these names are uncovered" for a workspace + with no export at all — which is the *common* case here, since a server + whose surface exists only as source is the population this input was built + for. The caller renders a shortfall as "an MCP tool export is also present + and does not name them", so every source-only server, `mongodb-js`, + `grafana` and `github` included, published a claim about a file that does + not exist into the evidence a human reads when deciding whether to adopt. + """ + + workspace = _grafana_shaped(tmp_path) + found = discover_mcp_server_source(workspace, files=_inventory(workspace)) + + assert found.detected + assert not any("export is also present" in line for line in found.evidence) + + def test_a_partial_export_does_not_erase_the_rest_of_one_server(tmp_path): """Same rule inside a single server: cover the surface or stand aside.""" diff --git a/tests/test_zero_install_detector.py b/tests/test_zero_install_detector.py index f4e211fc..7578dd65 100644 --- a/tests/test_zero_install_detector.py +++ b/tests/test_zero_install_detector.py @@ -29,7 +29,10 @@ import pytest from agents_shipgate.cli.discovery import detect_workspace +from agents_shipgate.cli.discovery import mcp_source as mcp_source_discovery +from agents_shipgate.inputs import mcp_server_source from agents_shipgate.inputs.codex_plugin import resolve_local_codex_marketplace_roots +from tests.mcp_idiom_corpus import ESCAPE_CASES, SCANNABLE_PATHS, SOURCE_CASES REPO_ROOT = Path(__file__).resolve().parent.parent SCRIPT_PATH = REPO_ROOT / "tools" / "shipgate-detect.py" @@ -126,26 +129,24 @@ def test_script_does_not_claim_drop_in_parity(script_module): def test_framework_vocabulary_names_every_cli_omission(script_module): """Every framework the CLI can report is either here or a named omission. - The parity test above compares the two on ``samples/``, which is only as - strong as the fixtures: a detection the CLI gains and the script does not - is invisible to it until a sample exercises the difference. That is exactly - what happened with ``mcp_server_source`` (#431) — the CLI reads an MCP - server's tool names out of TypeScript or Go registration sites, no sample - contains one, and the script goes on reporting a repository like - ``mongodb-js/mongodb-mcp-server`` as *not an agent project*. - - So the omission is written down instead of discovered. Adding a detection - to the CLI now fails here until it is either ported or listed, and the list - is the thing a reader can check against the script's own documented - simplifications. + The parity sweep compares the two on ``samples/``, which is only as strong + as the fixtures: a detection the CLI gains and the script does not is + invisible to it until a sample exercises the difference. That is exactly + what happened with ``mcp_server_source`` (#431) — the CLI learned to read + an MCP server's tool names out of TypeScript or Go registration sites, no + sample contained one, and the script went on reporting + ``mongodb-js/mongodb-mcp-server`` as *not an agent project* for a whole + release. #485 ported it, and ``known_omissions`` is empty again. + + Keep it that way, or write the next omission down here rather than leaving + it to be discovered on someone's repository. A listed omission also has to + be legible in the script itself: a reader of the file must be able to find + it without reading this test. """ from agents_shipgate.cli.discovery.signals import _initial_framework_scores - # Documented, deliberate, and filed as #485. Porting the reader means a - # second implementation of the load-bearing matcher, which needs its own - # increment and a conformance corpus shared with the package. - known_omissions = {"mcp_server_source"} + known_omissions: set[str] = set() cli = set(_initial_framework_scores()) script = set(script_module.FRAMEWORKS) @@ -166,6 +167,25 @@ def test_framework_vocabulary_names_every_cli_omission(script_module): ) +def test_conventional_directory_frameworks_match_the_cli(script_module): + """A conventional directory is weak evidence for the same list on both sides. + + Not every framework: ``mcp_server_source``'s evidence is already a + conjunction, so a ``tools/`` directory adds nothing it does not have, and + adding it would carry the published confidence to ``high`` for a route the + engine caps at ``medium``. That exclusion never changes which frameworks + fire, so no parity assertion over a workspace can see it — which is why it + is pinned by value here. + """ + + from agents_shipgate.cli.discovery.signals import CONVENTIONAL_DIR_FRAMEWORKS + + assert ( + tuple(script_module.CONVENTIONAL_DIR_FRAMEWORKS) + == CONVENTIONAL_DIR_FRAMEWORKS + ) + + def test_script_emits_canonical_top_level_keys(script_module): """The script's JSON output must carry the same top-level keys as DetectResult, plus ``script_version`` to distinguish it from the @@ -206,23 +226,31 @@ def _write_skipped_fixture_signals(root: Path) -> None: (plugin / "plugin.json").write_text("{}", encoding="utf-8") -@pytest.mark.parametrize("sample_dir", _sample_dirs(), ids=_sample_ids()) -def test_script_verdict_matches_cli(script_module, sample_dir): - """Structural parity: for every sample, the zero-install script - must agree with the canonical CLI on (a) ``is_agent_project``, - (b) the set of fired frameworks, (c) the set of suggested-source - types and paths, (d) the set of excluded-source types and paths, - and (e) workspace-signals keys.""" - if sample_dir.name in SCRIPT_PARITY_GAPS: - pytest.skip( - f"{sample_dir.name}: zero-install script parity not yet implemented " - "(see SCRIPT_PARITY_GAPS)." - ) - script_result = script_module.detect(sample_dir) - cli_result = detect_workspace(sample_dir.resolve()).model_dump(mode="json") +def _framework(result: dict[str, Any], framework: str) -> dict[str, Any] | None: + return next( + (item for item in result["frameworks"] if item["type"] == framework), None + ) + + +def _assert_detect_parity(script_module, workspace: Path, label: str) -> None: + """Structural parity on one workspace: the script must agree with the + canonical CLI on (a) ``is_agent_project``, (b) the set of fired + frameworks, (c) the set of suggested-source types and paths, (d) the set + of excluded-source types and paths, and (e) workspace-signals keys. + + One function, called by the ``samples/`` sweep and by the constructed + workspaces below. A second comparison written beside this one would be a + weaker comparison: the route added in #485 is exercised by workspaces no + sample has, and pinning it against a hand-written subset of these + assertions is how the two detectors would agree on everything anybody + checked and differ everywhere else. + """ + + script_result = script_module.detect(workspace) + cli_result = detect_workspace(workspace.resolve()).model_dump(mode="json") assert script_result["is_agent_project"] == cli_result["is_agent_project"], ( - f"{sample_dir.name}: is_agent_project diverged " + f"{label}: is_agent_project diverged " f"(script={script_result['is_agent_project']}, " f"cli={cli_result['is_agent_project']})." ) @@ -230,11 +258,48 @@ def test_script_verdict_matches_cli(script_module, sample_dir): script_frameworks = sorted(f["type"] for f in script_result["frameworks"]) cli_frameworks = sorted(f["type"] for f in cli_result["frameworks"]) assert script_frameworks == cli_frameworks, ( - f"{sample_dir.name}: framework set diverged " + f"{label}: framework set diverged " f"(script={script_frameworks!r}, cli={cli_frameworks!r}). " "The script's scoring rules must match cli/discovery/signals.py." ) + # `mcp_server_source` is the one framework whose score and evidence are + # pinned as well as its presence. The looseness the rest of this contract + # grants — ±0.5 on the score, descriptive rather than byte-identical + # strings — was granted to detections scored from many heuristic signals + # and described in the script's own words. This one has exactly two + # scoring inputs, a resolved registration and a declared dependency, and + # its lines are rendered by a function ported verbatim; there is nothing + # here for the script to paraphrase. + # + # Both halves are load-bearing and invisible to every other assertion. The + # dependency is what carries the published label to `medium` rather than to + # the weakest thing discovery can say, and the framework fires either way. + # And the conditional lines are claims, not prose: "61 tools" without "and + # 3 more this reader cannot name" is the over-claim this whole input exists + # to avoid, and a detector that dropped it would agree on the verdict and + # publish a different fact about it (#485). + script_mcp = _framework(script_result, mcp_server_source.SOURCE_TYPE) + cli_mcp = _framework(cli_result, mcp_server_source.SOURCE_TYPE) + assert (script_mcp is None) == (cli_mcp is None) + if script_mcp is not None and cli_mcp is not None: + assert ( + script_mcp["score"], + script_mcp["confidence"], + script_mcp["evidence"], + ) == ( + cli_mcp["score"], + cli_mcp["confidence"], + cli_mcp["evidence"], + ), ( + f"{label}: the MCP registration route reported a different " + f"score/confidence/evidence " + f"(script={script_mcp['score']}/{script_mcp['confidence']}/" + f"{script_mcp['evidence']}, " + f"cli={cli_mcp['score']}/{cli_mcp['confidence']}/" + f"{cli_mcp['evidence']})." + ) + script_sources = sorted( (s["type"], s["path"]) for s in script_result["suggested_sources"] ) @@ -242,10 +307,25 @@ def test_script_verdict_matches_cli(script_module, sample_dir): (s["type"], s["path"]) for s in cli_result["suggested_sources"] ) assert script_sources == cli_sources, ( - f"{sample_dir.name}: suggested_sources diverged " + f"{label}: suggested_sources diverged " f"(script={script_sources!r}, cli={cli_sources!r})." ) + assert sorted( + (s["path"], s["reason"]) + for s in script_result["excluded_sources"] + if s["type"] == mcp_server_source.SOURCE_TYPE + ) == sorted( + (s["path"], s["reason"]) + for s in cli_result["excluded_sources"] + if s["type"] == mcp_server_source.SOURCE_TYPE + ), ( + f"{label}: the withheld MCP registration route was explained " + "differently. The reason is the only thing a reader gets when a route " + "disappears, and a route that vanishes without one is " + "indistinguishable from one nobody implemented." + ) + script_excluded = sorted( (s["type"], s["path"]) for s in script_result["excluded_sources"] ) @@ -253,7 +333,7 @@ def test_script_verdict_matches_cli(script_module, sample_dir): (s["type"], s["path"]) for s in cli_result["excluded_sources"] ) assert script_excluded == cli_excluded, ( - f"{sample_dir.name}: excluded_sources diverged " + f"{label}: excluded_sources diverged " f"(script={script_excluded!r}, cli={cli_excluded!r}). " "The script's stdlib parse probe must reject the same JSON " "candidates as cli/discovery/artifacts.py:probe_suggested_source." @@ -266,12 +346,12 @@ def test_script_verdict_matches_cli(script_module, sample_dir): (s["mode"], s["path"]) for s in cli_result["codex_plugin_candidates"] ) assert script_codex == cli_codex, ( - f"{sample_dir.name}: codex_plugin_candidates diverged " + f"{label}: codex_plugin_candidates diverged " f"(script={script_codex!r}, cli={cli_codex!r})." ) assert script_result["agent_scope"] == cli_result["agent_scope"], ( - f"{sample_dir.name}: agent_scope diverged " + f"{label}: agent_scope diverged " f"(script={script_result['agent_scope']!r}, " f"cli={cli_result['agent_scope']!r}). An agent that consults the " "zero-install path must not adopt a scope the CLI refuses." @@ -280,7 +360,7 @@ def test_script_verdict_matches_cli(script_module, sample_dir): script_result["python_parse_truncated"] == cli_result["python_parse_truncated"] ), ( - f"{sample_dir.name}: python_parse_truncated diverged " + f"{label}: python_parse_truncated diverged " f"(script={script_result['python_parse_truncated']!r}, " f"cli={cli_result['python_parse_truncated']!r}). It is the guard every " "whole-workspace negative is gated on, `is_agent_project: false` " @@ -291,7 +371,7 @@ def test_script_verdict_matches_cli(script_module, sample_dir): script_result["agent_scope_truncated"] == cli_result["agent_scope_truncated"] ), ( - f"{sample_dir.name}: agent_scope_truncated diverged " + f"{label}: agent_scope_truncated diverged " f"(script={script_result['agent_scope_truncated']!r}, " f"cli={cli_result['agent_scope_truncated']!r}). It says whether " "agent_project_candidates enumerates the workspace or only the part " @@ -307,14 +387,14 @@ def test_script_verdict_matches_cli(script_module, sample_dir): for c in cli_result["agent_project_candidates"] ) assert script_projects == cli_projects, ( - f"{sample_dir.name}: agent_project_candidates diverged " + f"{label}: agent_project_candidates diverged " f"(script={script_projects!r}, cli={cli_projects!r})." ) cli_signals = cli_result["workspace_signals"] script_signals = script_result["workspace_signals"] assert set(script_signals) == set(cli_signals), ( - f"{sample_dir.name}: workspace_signals keys diverged " + f"{label}: workspace_signals keys diverged " f"(script={set(script_signals)!r}, cli={set(cli_signals)!r})." ) # Keys alone let the same named field mean two different things. When the @@ -325,14 +405,14 @@ def test_script_verdict_matches_cli(script_module, sample_dir): # they are compared by value. for key in ("has_prompts_dir", "has_tools_dir", "conventional_dirs"): assert script_signals[key] == cli_signals[key], ( - f"{sample_dir.name}: workspace_signals[{key!r}] diverged " + f"{label}: workspace_signals[{key!r}] diverged " f"(script={script_signals[key]!r}, cli={cli_signals[key]!r}). " "The script must mirror cli/discovery/signals.py:" "_conventional_dir_locations exactly." ) assert script_result["agent_name_candidates"] == cli_result["agent_name_candidates"], ( - f"{sample_dir.name}: agent_name_candidates diverged.\n" + f"{label}: agent_name_candidates diverged.\n" f"script={script_result['agent_name_candidates']!r}\n" f"cli={cli_result['agent_name_candidates']!r}\n" "The ranking decides which agent the generated manifest declares as " @@ -342,6 +422,16 @@ def test_script_verdict_matches_cli(script_module, sample_dir): ) +@pytest.mark.parametrize("sample_dir", _sample_dirs(), ids=_sample_ids()) +def test_script_verdict_matches_cli(script_module, sample_dir): + if sample_dir.name in SCRIPT_PARITY_GAPS: + pytest.skip( + f"{sample_dir.name}: zero-install script parity not yet implemented " + "(see SCRIPT_PARITY_GAPS)." + ) + _assert_detect_parity(script_module, sample_dir, sample_dir.name) + + @pytest.mark.parametrize("sample_dir", _sample_dirs(), ids=_sample_ids()) def test_script_finds_at_least_one_python_file_when_cli_does( script_module, sample_dir @@ -1085,3 +1175,542 @@ def test_script_locates_nested_conventional_dirs_like_the_cli( "awslabs/billing_cost_management_mcp_server/tools", ] assert cli_signals["has_tools_dir"] is True + + +# --- The two MCP registration readers --------------------------------------- +# +# `tools/shipgate-detect.py` carries a stdlib-only port of +# `agents_shipgate.inputs.mcp_idioms` (#485). That is a second implementation +# of a load-bearing matcher, which is the recurring bug class in this +# repository, so it is not held to the CLI's answers by inspection: every case +# either reader has ever been asked about lives once in +# `tests/mcp_idiom_corpus.py`, and the tests below drive all of it through +# both. Add a case there, never in one test file — a case added to one is a +# case the other reader was never asked. + + +def _site_fields(site: Any) -> dict[str, Any]: + """One site as plain data, for comparison across the two implementations. + + Every field, ``span`` included. The span is not cosmetic: containment of + one span in another is what decides whether a wrapper call reports a second + omission for a tool its own argument already named, so two readers with + matching names and different spans disagree about the exclusion ledger. + """ + + return { + "idiom": site.idiom, + "name": site.name, + "line": site.line, + "column": site.column, + "span": tuple(site.span), + "description": site.description, + "operation_type": site.operation_type, + "unresolved_reason": site.unresolved_reason, + } + + +@pytest.mark.parametrize( + "case", SOURCE_CASES, ids=[case.case for case in SOURCE_CASES] +) +def test_both_readers_resolve_the_corpus_identically(script_module, case): + """Same sites, same fields, same anomalies — from the same source text.""" + + from agents_shipgate.inputs import mcp_idioms + + cli = mcp_idioms.scan_source(case.text, case.language) + script = script_module.scan_source(case.text, case.language) + + assert [_site_fields(site) for site in script.sites] == [ + _site_fields(site) for site in cli.sites + ], ( + f"{case.case}: the zero-install reader and " + "agents_shipgate.inputs.mcp_idioms resolved different registration " + "sites for the same source." + ) + assert script.anomalies == cli.anomalies, ( + f"{case.case}: the two readers disagree about whether this file could " + "be masked at all, which decides whether its surface is reported " + "complete or partial." + ) + + +@pytest.mark.parametrize( + "case", SOURCE_CASES, ids=[case.case for case in SOURCE_CASES] +) +def test_neither_reader_answers_differently_on_a_crlf_checkout(script_module, case): + """A Windows checkout is a supported one, and it changes every offset. + + Git for Windows translates line endings on checkout by default, so the + detector a maintainer curls onto their own repository is reading `\r\n` + source. The maskers work in offsets and the value tests skip `\r` before + looking for a terminator, so the answer should not move — but "should not" + is what a test is for, and this repository has lost a day to CRLF in a + corpus reader before. + + Names, omissions and anomalies rather than whole sites: the two carriage + returns per line legitimately shift every line's spans. The readers are + still compared to each other in full, because they see identical bytes. + """ + + from agents_shipgate.inputs import mcp_idioms + + crlf = case.text.replace("\n", "\r\n") + + def answer(result): + return ( + sorted(site.name for site in result.sites if site.name), + sorted( + site.unresolved_reason for site in result.sites if site.name is None + ), + result.anomalies, + ) + + lf = mcp_idioms.scan_source(case.text, case.language) + cli = mcp_idioms.scan_source(crlf, case.language) + script = script_module.scan_source(crlf, case.language) + + assert answer(cli) == answer(lf), ( + f"{case.case}: the CLI reader answers differently on a CRLF checkout" + ) + assert [_site_fields(site) for site in script.sites] == [ + _site_fields(site) for site in cli.sites + ], f"{case.case}: the two readers diverge on a CRLF checkout" + assert script.anomalies == cli.anomalies + + +@pytest.mark.parametrize( + ("path", "scannable"), SCANNABLE_PATHS, ids=[case[0] for case in SCANNABLE_PATHS] +) +def test_both_readers_open_the_same_files(script_module, path: str, scannable: bool): + """The path predicate decides which files are the surface at all. + + A disagreement here is a tool one detector can see and the other cannot, + before any masking happens — and it is the half of the contract that a + source-text corpus can never reach. + """ + + from agents_shipgate.inputs import mcp_idioms + + assert script_module.is_scannable_path(path) is scannable + assert mcp_idioms.is_scannable_path(path) is scannable + + +@pytest.mark.parametrize( + ("body", "language", "expected"), + ESCAPE_CASES, + ids=[f"{language}:{body}" for body, language, _expected in ESCAPE_CASES], +) +def test_both_readers_decode_escapes_identically( + script_module, body: str, language: str, expected: str | None +): + """A decoder that differs by one grammar publishes a name nobody serves. + + Go writes an octal escape as three digits, so ``MustTool("delete\\137all")`` + registers ``delete_all``; a JavaScript-shaped decoder produced + ``delete137all`` — the real action absent from the catalog and an id + nobody serves standing in for it. Two decoders make that failure possible + twice. + """ + + from agents_shipgate.inputs import mcp_idioms + + assert script_module.decode_literal(body, language) == expected + assert mcp_idioms.decode_literal(body, language) == expected + + +def test_the_script_implements_every_idiom_the_cli_registry_publishes(script_module): + """An idiom the CLI gains and the script does not is #485 happening again. + + The corpus sweep would catch it too — every idiom is required to have a + positive sample, and the sample would resolve on one side only — but the + failure would read as one mysterious case rather than as a missing idiom. + + Pinned from both sides, because a constant no code reads can describe a + reader it has drifted from: equal to the CLI registry, *and* equal to the + ids the script's own reader emits across the corpus — which is the whole + registry, since every idiom is required to have a positive sample. + """ + + from agents_shipgate.inputs.mcp_idioms import IDIOMS_BY_ID + + assert set(script_module.IDIOM_IDS) == set(IDIOMS_BY_ID) + + emitted = { + site.idiom + for case in SOURCE_CASES + for site in script_module.scan_source(case.text, case.language).sites + } + assert emitted == set(script_module.IDIOM_IDS) + + +def test_the_script_records_omissions_in_the_cli_vocabulary(script_module): + """A reason spelled differently is a second vocabulary for one event. + + Asserted over what the reader produces rather than over a constant listing + what it might: the script emits site-level reasons only — the file-level + ones (``file_too_large``, ``unreadable_file``) belong to the scan-time + adapter — so a copied constant would have pinned four tokens no code here + can reach. + """ + + from agents_shipgate.inputs.mcp_idioms import OMISSION_REASONS + + produced = { + site.unresolved_reason + for case in SOURCE_CASES + for site in script_module.scan_source(case.text, case.language).sites + if site.unresolved_reason is not None + } + assert produced, "the corpus no longer exercises a single unresolved site" + assert produced <= set(OMISSION_REASONS) + + +def test_the_script_mirrors_the_readers_shared_vocabulary(script_module): + """Constants that decide what either reader looks at, pinned by value. + + Each of these is a silent divergence rather than a loud one: a framework + package missing from one list withholds the whole route in that detector + and says nothing, and an omission reason spelled differently is a second + vocabulary for one event. + """ + + from agents_shipgate.inputs import mcp_idioms + + assert script_module.LANGUAGE_EXTENSIONS == mcp_idioms.LANGUAGE_EXTENSIONS + assert ( + script_module.TYPESCRIPT_FRAMEWORK_PACKAGES + == mcp_idioms.TYPESCRIPT_FRAMEWORK_PACKAGES + ) + assert script_module.GO_FRAMEWORK_MODULES == mcp_idioms.GO_FRAMEWORK_MODULES + assert script_module.SKIP_DIRECTORY_NAMES == mcp_idioms.SKIP_DIRECTORY_NAMES + assert script_module.TEST_DIRECTORY_NAMES == mcp_idioms.TEST_DIRECTORY_NAMES + assert script_module.PREFILTER_TOKEN == mcp_idioms.PREFILTER_TOKEN + assert script_module.TOOL_NAME_RE.pattern == mcp_idioms.TOOL_NAME_RE.pattern + assert script_module.MAX_SOURCE_FILE_BYTES == mcp_idioms.MAX_SOURCE_FILE_BYTES + assert script_module.MCP_SOURCE_TYPE == mcp_server_source.SOURCE_TYPE + assert ( + script_module.DEFAULT_MAX_SOURCE_FILES + == mcp_source_discovery.DEFAULT_MAX_SOURCE_FILES + ) + + +# --- The MCP source route, on workspaces no sample has ---------------------- +# +# `samples/mcp_source_only_server` pins the plain case through the sweep above. +# These are the branches around it, and every one of them is a place a route +# can be withheld: withholding is invisible in a verdict that was already +# `true`, so a detector that withheld for the wrong reason would look identical +# to one that did not. + + +def _write(root: Path, files: dict[str, str]) -> Path: + for name, body in files.items(): + target = root / name + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(body, encoding="utf-8") + return root + + +_TS_PACKAGE_JSON = '{"name": "srv", "dependencies": {"@modelcontextprotocol/sdk": "^1.0.0"}}' +_GO_MOD = "module example.com/srv\n\ngo 1.23\n\nrequire github.com/mark3labs/mcp-go v0.30.0\n" +_TS_REGISTRATION = 'server.registerTool("search_docs", { inputSchema: shape }, handler);\n' +_GO_REGISTRATION = 'var Incident = mcpgrafana.MustTool("update_incident", "Update", update)\n' + +_MCP_ROUTE_WORKSPACES: dict[str, dict[str, str]] = { + # The plain TypeScript route: a declared SDK dependency and a name + # resolved at a registration site. + "typescript_source_only": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + }, + # The same shape in Go, which is two of the three vendor servers this + # input exists for. + "go_source_only": { + "go.mod": _GO_MOD, + "pkg/incident.go": _GO_REGISTRATION, + }, + # No declared MCP dependency: a class of one's own spelling a field + # `toolName` is a coincidence until something says otherwise, so neither + # detector may offer the route. + "registrations_without_a_declared_dependency": { + "package.json": '{"name": "srv", "dependencies": {"express": "^4.0.0"}}', + "src/tools/search.ts": _TS_REGISTRATION, + }, + # The dependency is declared and nothing resolves: "uses MCP" is what + # every client can say, so it is not a tool surface. + "dependency_without_a_resolved_registration": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": "server.registerTool(NAME, { inputSchema: shape }, h);\n", + }, + # A test's fake tool is not the published surface. + "registrations_only_under_tests": { + "package.json": _TS_PACKAGE_JSON, + "src/__tests__/search.ts": _TS_REGISTRATION, + }, + # An export naming every registration wins: it is the server's own + # published contract, carries the input schemas this route does not read, + # and is high confidence against medium. The route is withheld and named + # in `excluded_sources`, never silently dropped. + "export_covers_every_registration": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + "mcp-tools.json": '{"tools": [{"name": "search_docs", "description": "d"}]}', + }, + # A partial export used to withhold the route anyway, which deleted the + # registrations it does not name. Both routes are suggested instead. + "export_covers_part_of_the_surface": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + "src/tools/index.ts": 'server.registerTool("list_docs", { inputSchema: s }, h);\n', + "mcp-tools.json": '{"tools": [{"name": "search_docs", "description": "d"}]}', + }, + # A wildcard export enumerates nothing, so it can never be shown to + # contain anything: the source route stands, and the evidence says an + # export is present that names none of these registrations. The shape is + # the one the CLI's own wildcard test uses. + "wildcard_export_contains_nothing": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + "mcp-tools.json": '{"wildcard": true, "tools": []}', + }, + # Two registration directories: the route is their common ancestor, which + # is the directory the adapter walks once the manifest points at it. + "registrations_in_two_directories": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + "src/admin/drop.ts": 'class T { static toolName = "drop-database"; }\n', + }, + # Both languages register in one repository — `mcp-grafana` is shaped this + # way — so the route is the ancestor of both and the evidence names the + # pair. A port that resolved one language's gate but not the other would + # still offer a route, just a narrower one. + "both_languages_register_tools": { + "package.json": _TS_PACKAGE_JSON, + "go.mod": _GO_MOD, + "internal/ts/search.ts": _TS_REGISTRATION, + "internal/go/incident.go": _GO_REGISTRATION, + }, + # Both gates open and only one language actually registers — the shape + # `grafana/mcp-grafana` has, where a `ui/` package declares an MCP + # dependency and every tool is in Go. The evidence names the language the + # tools were *read* in, not the languages the workspace declared; naming + # both would say a TypeScript surface exists that this reader never found. + "a_declared_language_that_registers_nothing": { + "package.json": _TS_PACKAGE_JSON, + "go.mod": _GO_MOD, + "pkg/incident.go": _GO_REGISTRATION, + "src/client.ts": "const client = new Client();\nawait client.callTool(x);\n", + }, + # An unresolved registration outside the route directory. The count is + # taken over the directory the route points at, so this one is *not* + # reported: naming a registration `scan` will never reach is the mirror of + # the over-claim the count exists to prevent. + "an_unresolved_site_outside_the_route": { + "package.json": _TS_PACKAGE_JSON, + "src/tools/search.ts": _TS_REGISTRATION, + "other/registry.ts": "server.registerTool(NAME, { inputSchema: s }, h);\n", + }, + # A single-file server registers at the workspace root, so the route is + # `"."` — its own branch in the unresolved-count rollup, and the one route + # path that also reaches `agent_project_candidates` as a bare workspace. + "registrations_at_the_workspace_root": { + "package.json": _TS_PACKAGE_JSON, + "server.ts": _TS_REGISTRATION + + "server.registerTool(DYNAMIC, { inputSchema: shape }, handler);\n", + }, +} + + +@pytest.mark.parametrize("case", sorted(_MCP_ROUTE_WORKSPACES), ids=sorted(_MCP_ROUTE_WORKSPACES)) +def test_script_and_cli_agree_on_the_mcp_source_route(script_module, tmp_path, case): + workspace = _write(tmp_path / case, _MCP_ROUTE_WORKSPACES[case]) + _assert_detect_parity(script_module, workspace, case) + + +def test_the_constructed_route_workspaces_actually_exercise_the_route(tmp_path): + """The parity assertions above pass on two detectors that both do nothing. + + A fixture set that never fires the route would agree perfectly and prove + nothing — the same vacuum `test_script_verdict_matches_cli` sat in for a + whole release. So the CLI's own answers are pinned here: which of these + workspaces offers the route, which withholds it and says why, and which + never had one. + """ + + routed: dict[str, str | None] = {} + excluded: dict[str, list[str]] = {} + for case, files in _MCP_ROUTE_WORKSPACES.items(): + workspace = _write(tmp_path / case, files) + result = detect_workspace(workspace.resolve()).model_dump(mode="json") + routed[case] = next( + ( + source["path"] + for source in result["suggested_sources"] + if source["type"] == mcp_server_source.SOURCE_TYPE + ), + None, + ) + excluded[case] = [ + source["path"] + for source in result["excluded_sources"] + if source["type"] == mcp_server_source.SOURCE_TYPE + ] + + assert routed == { + "typescript_source_only": "src/tools", + "go_source_only": "pkg", + "registrations_without_a_declared_dependency": None, + "dependency_without_a_resolved_registration": None, + "registrations_only_under_tests": None, + "export_covers_every_registration": None, + "export_covers_part_of_the_surface": "src/tools", + "wildcard_export_contains_nothing": "src/tools", + "registrations_in_two_directories": "src", + "registrations_at_the_workspace_root": ".", + "both_languages_register_tools": "internal", + "a_declared_language_that_registers_nothing": "pkg", + "an_unresolved_site_outside_the_route": "src/tools", + } + assert excluded == { + "typescript_source_only": [], + "go_source_only": [], + "registrations_without_a_declared_dependency": [], + "dependency_without_a_resolved_registration": [], + "registrations_only_under_tests": [], + "export_covers_every_registration": ["src/tools"], + "export_covers_part_of_the_surface": [], + "wildcard_export_contains_nothing": [], + "registrations_in_two_directories": [], + "registrations_at_the_workspace_root": [], + "both_languages_register_tools": [], + "a_declared_language_that_registers_nothing": [], + "an_unresolved_site_outside_the_route": [], + } + + +def test_script_and_cli_refuse_the_same_undecodable_source_file( + script_module, tmp_path +): + """One decoding contract across both detectors, not just one path predicate. + + Discovery decoding with ``errors="replace"`` was already a shipped defect + once: ``detect`` resolved a registration out of a file the scan-time loader + then refuses as ``unreadable_file``, so the route it named enumerated fewer + tools than it promised. A port that shares the path predicate and not the + read reintroduces exactly that, in the detector a cold maintainer runs + first. + + The undecodable bytes sit in a *comment*, with an ordinary registration + beside them. Bad bytes inside the name would make both readers agree for + the wrong reason — replacement characters fail the tool-name shape, so a + lenient decode and a strict refusal produce the same empty answer. + """ + + workspace = tmp_path / "mixed" + (workspace / "src" / "tools").mkdir(parents=True) + (workspace / "legacy").mkdir() + (workspace / "package.json").write_text(_TS_PACKAGE_JSON, encoding="utf-8") + (workspace / "src" / "tools" / "search.ts").write_text( + _TS_REGISTRATION, encoding="utf-8" + ) + # A lenient reader resolves `legacy_tool` here and widens the route's + # common ancestor from `src/tools` to the workspace root, which is what + # makes the difference visible in the verdict rather than only in a count. + (workspace / "legacy" / "broken.ts").write_bytes( + b"// \xff\xfe\n" b'server.registerTool("legacy_tool", {}, handler);\n' + ) + + _assert_detect_parity(script_module, workspace, "undecodable_source_file") + + cli_result = detect_workspace(workspace.resolve()).model_dump(mode="json") + assert [ + source["path"] + for source in cli_result["suggested_sources"] + if source["type"] == mcp_server_source.SOURCE_TYPE + ] == ["src/tools"], ( + "the undecodable file was read after all, so this fixture no longer " + "distinguishes a strict read from a lenient one" + ) + + +def test_script_and_cli_stop_at_the_same_source_file_cap(script_module, tmp_path): + """The cap, and the flag that reports it, describe the same walk in both. + + Unreachable through ``detect`` without 1500 source files, and it is not a + cosmetic bound: which files are read decides which names are found, so two + detectors capping differently would name different tools and point at + different route directories on any repository large enough to hit it. The + CLI shipped a version that reported ``truncated`` while reading every file; + the port has its own copy of that slice. + """ + + workspace = tmp_path / "many" + (workspace / "src").mkdir(parents=True) + (workspace / "package.json").write_text(_TS_PACKAGE_JSON, encoding="utf-8") + for index in range(6): + (workspace / "src" / f"t{index}.ts").write_text( + f'server.registerTool("tool_{index}", {{}}, handler);\n', encoding="utf-8" + ) + + from agents_shipgate.cli.discovery.artifacts import _candidate_files + + for cap in (2, 6, 10): + cli = mcp_source_discovery.discover_mcp_server_source( + workspace, + files=_candidate_files(workspace), + max_source_files=cap, + ) + script = script_module._discover_mcp_server_source( + workspace, script_module._inventory(workspace), [], cap + ) + assert (script.path, script.tool_names, script.truncated) == ( + cli.path, + cli.tool_names, + cli.truncated, + ), f"the two detectors read different files at max_source_files={cap}" + + # And the fixture is only meaningful because the cap actually binds at 2. + assert mcp_source_discovery.discover_mcp_server_source( + workspace, files=_candidate_files(workspace), max_source_files=2 + ).truncated is True + + +def test_an_export_past_the_size_bound_does_not_withhold_the_source_route( + script_module, tmp_path, monkeypatch +): + """The bound is the loader's, so the outcome has to be the loader's too. + + ``load_mcp_tools`` refuses an input over 10 MB before parsing it, so on the + CLI side an oversized export is excluded at the probe and never reaches the + containment test — the source route stands. The port reads the file itself, + so without the same bound it would read an arbitrarily large JSON out of an + unknown repository *and* withhold a route the CLI keeps. + + Driven by lowering the bound rather than by writing a 10 MB fixture; the + file being over it is the whole condition. + """ + + workspace = _write( + tmp_path / "big-export", _MCP_ROUTE_WORKSPACES["export_covers_every_registration"] + ) + + withheld = script_module.detect(workspace) + assert [ + source["path"] + for source in withheld["excluded_sources"] + if source["type"] == script_module.MCP_SOURCE_TYPE + ] == ["src/tools"], "the fixture no longer withholds the route at all" + + monkeypatch.setattr(script_module, "MAX_STRUCTURED_FILE_BYTES", 8) + kept = script_module.detect(workspace) + assert [ + source["path"] + for source in kept["suggested_sources"] + if source["type"] == script_module.MCP_SOURCE_TYPE + ] == ["src/tools"] + assert not [ + source + for source in kept["excluded_sources"] + if source["type"] == script_module.MCP_SOURCE_TYPE + ] diff --git a/tools/shipgate-detect.py b/tools/shipgate-detect.py index 7567cbdd..56e847ff 100644 --- a/tools/shipgate-detect.py +++ b/tools/shipgate-detect.py @@ -48,14 +48,6 @@ - No ``diagnostics[]`` / ``next_actions[]`` (the diagnostic engine is not in scope for stdlib-only / zero-install). -- **No ``mcp_server_source`` detection.** The installed CLI reads an MCP - server's tool names out of TypeScript or Go registration sites through a - built-in idiom registry (#431); this script does not, so a repository whose - tool surface exists only as code is reported here as *not* an agent project - while the CLI reports it as one. That is the largest divergence in this list - and the only one that changes ``is_agent_project``. No sample exercises it - today, so the parity test cannot see it; ``test_framework_vocabulary_names_every_cli_omission`` - pins it instead, and it is filed as #485. - ``agent_scope`` / ``agent_scope_truncated`` / ``python_parse_truncated`` / ``agent_project_candidates[]`` are carried, and the contract test pins them against the CLI: an agent that @@ -70,6 +62,15 @@ guards against — ``mcpServers``-style host configs — is always JSON, so the probe is exact where it matters. +``mcp_server_source`` — an MCP server whose tool surface exists only as +TypeScript or Go registration sites (#431) — **is** detected here, through a +port of the CLI's masking reader and its idiom registry (#485). That port is a +second implementation of a load-bearing matcher, so it is held to the CLI's +answers by a shared conformance corpus rather than by inspection: every +positive sample, the whole adversarial sweep, the path predicate and both +escape grammars in ``tests/mcp_idiom_corpus.py`` are driven through both +readers and compared site by site, span included. + The verdict, detected framework set, suggested/excluded source split, and the ranked ``agent_name_candidates`` all match. The name ranking is pinned rather than simplified: it decides which agent a manifest declares as the @@ -87,10 +88,11 @@ import subprocess import sys import threading -from pathlib import Path +from dataclasses import dataclass, field +from pathlib import Path, PurePosixPath from typing import Any -SCRIPT_VERSION = "0.4.0" +SCRIPT_VERSION = "0.5.0" MAX_STRUCTURED_FILE_BYTES = 10 * 1024 * 1024 # Matches ``detect_workspace``'s ``max_python_files``. The bound is on # parses, not on the inventory: capping the inventory lets an asset-heavy @@ -127,9 +129,31 @@ "conductor": ("conductor-client", "conductor-server", "conductor-oss"), "openai_api": (), } +#: The tool-source type the MCP registration-site route suggests, and the +#: framework key it scores under. Mirrors +#: ``agents_shipgate.inputs.mcp_server_source.SOURCE_TYPE``. +MCP_SOURCE_TYPE = "mcp_server_source" FRAMEWORKS = ( "langchain", "crewai", "google_adk", "anthropic", "openai_agents_sdk", "n8n", "conductor", "openai_api", + # Not a Python framework and scored from neither the AST pass nor a + # filename glob: it is the workspace's own TypeScript or Go registration + # sites, scored by `_discover_mcp_server_source` (#431, ported by #485). + MCP_SOURCE_TYPE, +) +#: The frameworks a conventional directory is weak evidence for. Deliberately +#: not every entry in ``FRAMEWORKS`` — mirrors +#: ``signals.CONVENTIONAL_DIR_FRAMEWORKS``, and pinned against it. +#: +#: ``mcp_server_source`` is the one absentee. Its evidence is already a +#: conjunction — a declared MCP dependency *and* a tool name resolved at a +#: registration site — so a ``tools/`` directory adds nothing it does not +#: already have, and adding it would let two conventional directories carry +#: the published confidence to ``high`` for a route the engine caps at +#: ``medium``. +CONVENTIONAL_DIR_FRAMEWORKS = ( + "langchain", "crewai", "google_adk", "anthropic", + "openai_agents_sdk", "n8n", "conductor", "openai_api", ) OPENAPI_PATTERNS = ( "*openapi*.yaml", "*openapi*.yml", "*openapi*.json", @@ -677,6 +701,1614 @@ def _probe_openapi(data: Any) -> str | None: return None +# --- MCP registration idioms (mirror of inputs/mcp_idioms.py) --------------- +# +# Most MCP servers never emit their tool surface: `mongodb-js/mongodb-mcp-server` +# and `grafana/mcp-grafana` publish `drop-database`, `delete-many` and +# `update_incident` and commit no export at all. What every one of them does do +# is write the tool's name as a **string literal at a registration site**. The +# installed CLI reads that literal through a built-in registry of named idioms +# (`agents_shipgate.inputs.mcp_idioms`, #431); until #485 this script did not, +# so the documented first-contact detector answered "Stop, not an agent +# project" on exactly the repositories the installed CLI had just learned to +# read. +# +# This is a second implementation of a load-bearing matcher, which is the +# recurring bug class in this repository. What makes it affordable is that it +# is not allowed to become a *different* one: `tests/mcp_idiom_corpus.py` holds +# one conformance corpus — every idiom's positive sample, the whole adversarial +# sweep, the path predicate's cases and both escape grammars — and +# `tests/test_zero_install_detector.py` drives it through both readers, +# comparing every field of every site including its span. Neither reader can +# change its answer on any case either of them has ever been asked about +# without the other following. +# +# Reading is done over a **masked** copy of the source, in which comments and +# string bodies have been overwritten. A registration site can therefore never +# be found inside a comment or inside another string, and a name is a name only +# when the masking pass recorded a real literal at that offset. + +#: File suffixes each language's idioms are read from. TypeScript's list covers +#: JavaScript too. ``.tsx``/``.jsx`` are deliberately absent: JSX puts prose in +#: code position, so an apostrophe in ``

don't

`` would open a string that +#: never closes and hold a whole repository's surface at partial. +LANGUAGE_EXTENSIONS: dict[str, tuple[str, ...]] = { + "typescript": (".ts", ".mts", ".cts", ".js", ".mjs", ".cjs"), + "go": (".go",), +} + +#: Declared-dependency tokens that establish an MCP framework for a language. +#: The provenance gate: an idiom hit in a repository that declares no MCP +#: dependency is a coincidence of spelling until something says otherwise. +TYPESCRIPT_FRAMEWORK_PACKAGES: tuple[str, ...] = ( + "@modelcontextprotocol/", + "fastmcp", + "mcp-framework", + "@mcp-ui/", + "xmcp", +) +GO_FRAMEWORK_MODULES: tuple[str, ...] = ( + "github.com/modelcontextprotocol/go-sdk", + "github.com/mark3labs/mcp-go", + "github.com/metoro-io/mcp-golang", + "github.com/thinkinaixyz/go-mcp", + "github.com/ktr0731/go-mcp", +) + +#: Directory names never walked for registration sites. Distinct from this +#: script's own inventory-level ``SKIP_DIRS`` on purpose: this is the CLI +#: reader's list, and the two detectors have to skip the same directories *for +#: registrations* whatever each one's inventory already dropped. +SKIP_DIRECTORY_NAMES: frozenset[str] = frozenset( + { + ".git", ".hg", ".svn", ".next", ".nuxt", ".turbo", ".venv", + "__pycache__", "bin", "build", "coverage", "dist", "node_modules", + "obj", "out", "target", "vendor", "venv", + } +) + +#: Path segments whose files declare tools for a test, not for the server. +TEST_DIRECTORY_NAMES: frozenset[str] = frozenset( + { + "__mocks__", "__tests__", "e2e", "fixtures", "test", "test-fixtures", + "testdata", "tests", + } +) +_TEST_FILE_SUFFIXES: tuple[str, ...] = ( + "_test.go", + ".test.ts", ".test.js", ".test.mts", ".test.mjs", + ".spec.ts", ".spec.js", ".spec.mts", ".spec.mjs", +) + +#: Every idiom's pattern requires these four characters, in some case. A file +#: that does not contain them cannot hold a registration, so ``scan_source`` +#: answers it without masking. It lives *inside* ``scan_source`` rather than at +#: the call sites: a caller's own copy is a second, weaker matcher, and the one +#: written against the trigger catalog's diff tokens missed +#: ``public static readonly toolName``. +PREFILTER_TOKEN = "tool" + +#: The shape a tool name has to have to be read as one. +TOOL_NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._:-]{0,127}$") + +#: The largest source file this reader opens. Deliberately below the input +#: loader's 10 MB bound, so an oversized file is recorded as "too large" +#: rather than as a decoding failure. +MAX_SOURCE_FILE_BYTES = 8 * 1024 * 1024 + +#: The idioms this reader implements, by id. Pinned from both sides by the +#: conformance test: equal to the CLI registry's ids — an idiom the CLI gains +#: and this script does not is the #485 divergence happening again — and +#: covering every id ``scan_source`` actually emits, so the constant cannot +#: describe a reader it has drifted from. +IDIOM_IDS: frozenset[str] = frozenset( + { + "ts_static_tool_name", + "ts_sdk_register_tool", + "go_must_tool", + "go_new_tool", + "go_tool_struct", + } +) + + +@dataclass(frozen=True) +class RegistrationSite: + """One registration this reader found, resolved or not. + + ``span`` is the byte range of the construct that matched — the whole call + including its argument list, or the whole composite literal, never a + lookup scope. Containment of one span in another is what lets a wrapper + call whose own first argument is not a literal + (``NewTool(meta, mcp.Tool{Name: "issue_read"})``) stay silent instead of + reporting an omission for a tool that was, in fact, named. + """ + + idiom: str + name: str | None + line: int + column: int + span: tuple[int, int] + description: str | None = None + operation_type: str | None = None + unresolved_reason: str | None = None + + +@dataclass(frozen=True) +class SourceScanResult: + """What one file yielded. + + ``anomalies`` are masking failures. They are separate from an unresolved + site because they are a fact about the *file*: past the anomaly this reader + cannot tell code from content, so a site it did not find there proves + nothing. + """ + + sites: tuple[RegistrationSite, ...] = () + anomalies: tuple[str, ...] = () + + +def language_for_path(path: Any) -> str | None: + """The language whose idioms apply to ``path``, or ``None``.""" + suffix = PurePosixPath(str(path)).suffix.lower() + for language, extensions in LANGUAGE_EXTENSIONS.items(): + if suffix in extensions: + return language + return None + + +def is_scannable_path(relative_path: Any) -> bool: + """Whether a workspace-relative path is read for registration sites. + + One predicate on the CLI side, used by both its adapter's walk and its + discovery probe. This is its mirror, and the pair that disagrees is the + pair where one detector promises tools the other refuses to enumerate. + """ + path = PurePosixPath(str(relative_path).replace("\\", "/")) + if language_for_path(path) is None: + return False + parts = path.parts + if any(part in SKIP_DIRECTORY_NAMES for part in parts): + return False + if any(part.lower() in TEST_DIRECTORY_NAMES for part in parts[:-1]): + return False + name = path.name.lower() + return not name.endswith(_TEST_FILE_SUFFIXES) + + +# Masking. Comments become spaces so a token cannot span the hole they leave; +# string literals become NULs so a literal's *position* stays findable while +# its content can never be matched as code. +_COMMENT_FILL = " " +_STRING_FILL = "\x00" + +# Characters after which a `/` opens a regular expression rather than dividing. +_REGEX_PRECEDING_CHARS = frozenset("(,=:[!&|?{};+-*%~^<>") +#: Keywords whose parenthesised condition can be followed directly by a regex +#: that begins the statement's body. `)` alone cannot decide: `foo(a) / 2` +#: divides and `if (a) /re/.test(b)` does not. +_REGEX_PRECEDING_STATEMENTS = frozenset({"if", "for", "while", "switch", "catch", "with"}) +_REGEX_PRECEDING_WORDS = frozenset( + { + "return", "typeof", "instanceof", "in", "of", "new", "delete", "void", + "throw", "case", "do", "else", "yield", "await", + } +) + +#: The only characters that can begin a comment or a string in either language. +#: The masking loop jumps between them instead of visiting every character. +_INTERESTING = re.compile(r"""[/'"`]""") + +#: Escapes both languages spell the same way and mean the same thing. +_SHARED_ESCAPES = { + "n": "\n", "t": "\t", "r": "\r", "b": "\b", "f": "\f", "v": "\v", + "\\": "\\", "'": "'", '"': '"', +} +#: JavaScript adds a backtick, ``\0`` for NUL, and a line continuation. +_TYPESCRIPT_ESCAPES = {**_SHARED_ESCAPES, "`": "`"} +#: Go adds the bell and has no line continuation and no bare ``\0``. +_GO_ESCAPES = {**_SHARED_ESCAPES, "a": "\a"} + +#: The line terminators a backslash can continue a line across. ``\r`` is here +#: because a CRLF checkout spells the same continuation with two characters, +#: and JavaScript reads both files identically — so a reader that lost the +#: registration on one of them would answer "not an agent project" for a +#: line-ending translation (#485 review). +_TYPESCRIPT_LINE_TERMINATORS = frozenset("\n\r") +#: Go adds the bell and has no line continuation and no bare ``\0``. +_GO_ESCAPES = {**_SHARED_ESCAPES, "a": "\a"} + + +_HEX_DIGITS = frozenset("0123456789abcdefABCDEF") +_OCTAL_DIGITS = frozenset("01234567") + + +@dataclass(frozen=True) +class MaskedSource: + """``text`` with comments and string bodies overwritten. + + ``literals`` maps the offset of a string literal's opening quote to its + decoded value (``None`` when the literal is not a constant) and the offset + just past its closing quote. The end offset is recorded rather than + recovered by scanning the fill characters, because masking preserves + newlines: a multi-line template literal's fill run stops at its first line + break, and a caller asking what follows the literal would be looking + inside it. + """ + + text: str + masked: str + literals: dict[int, tuple[str | None, int]] + anomalies: tuple[str, ...] + + def skip_space(self, index: int) -> int: + length = len(self.masked) + while index < length and self.masked[index].isspace(): + index += 1 + return index + + def literal_at(self, index: int) -> tuple[bool, str | None, int]: + """Resolve a string literal starting at ``index`` (whitespace skipped).""" + start = self.skip_space(index) + record = self.literals.get(start) + if record is None: + return False, None, start + value, end = record + return True, value, end + + def line_column(self, index: int) -> tuple[int, int]: + prefix = self.text[:index] + line = prefix.count("\n") + 1 + column = index - (prefix.rfind("\n") + 1) + 1 + return line, column + + +def mask_source(text: str, language: str) -> MaskedSource: + """Overwrite comments and string bodies, recording every string literal.""" + if language == "go": + return _mask_go(text) + return _mask_typescript(text) + + +def decode_literal(body: str, language: str) -> str | None: + """The literal's value, or ``None`` when it cannot be decoded exactly. + + Escape grammars are per language, and one decoder shared between them is a + silent mistranslation rather than a parse error: Go writes an octal escape + as three digits, so ``MustTool("delete\\137all", …)`` registers + ``delete_all`` and a JavaScript-shaped decoder produced ``delete137all`` — + the real action absent and an id nobody serves in its place. Anything + either grammar does not define is refused, and a refusal becomes a + recorded omission instead of a guessed name. + """ + if "\\" not in body: + return body + if language == "go": + return _decode_go(body) + return _decode_typescript(body) + + +def _hex_value(body: str, start: int, width: int) -> int | None: + digits = body[start : start + width] + if len(digits) != width or any(char not in _HEX_DIGITS for char in digits): + return None + return int(digits, 16) + + +def _decode_typescript(body: str) -> str | None: + out: list[str] = [] + index = 0 + length = len(body) + while index < length: + char = body[index] + if char != "\\": + out.append(char) + index += 1 + continue + if index + 1 >= length: + return None + marker = body[index + 1] + if marker in _TYPESCRIPT_LINE_TERMINATORS: + # A LineContinuation contributes nothing to the value. CRLF is one + # terminator sequence: reading it as `\r` plus a stray line break + # both mangles the value and, in the scanner, ends the string. + index += 3 if marker == "\r" and body[index + 2 : index + 3] == "\n" else 2 + continue + if marker in _TYPESCRIPT_ESCAPES: + out.append(_TYPESCRIPT_ESCAPES[marker]) + index += 2 + continue + if marker == "0" and (index + 2 >= length or body[index + 2] not in "0123456789"): + out.append("\0") + index += 2 + continue + if marker == "x": + value = _hex_value(body, index + 2, 2) + if value is None: + return None + out.append(chr(value)) + index += 4 + continue + if marker == "u": + if index + 2 < length and body[index + 2] == "{": + close = body.find("}", index + 3) + digits = body[index + 3 : close] if close != -1 else "" + if not digits or any(char not in _HEX_DIGITS for char in digits): + return None + point = int(digits, 16) + if point > 0x10FFFF: + return None + out.append(chr(point)) + index = close + 1 + continue + value = _hex_value(body, index + 2, 4) + if value is None: + return None + out.append(chr(value)) + index += 6 + continue + if marker.isdigit(): + # Legacy octal (`\1`-`\7`) is a syntax error under `use strict` + # and in a template literal, and octal *elsewhere*; `\8`/`\9` are + # their own special case. Which one a file means depends on a mode + # this reader does not track, so it refuses rather than pick. + return None + out.append(marker) + index += 2 + return "".join(out) + + +def _decode_go(body: str) -> str | None: + out: list[str] = [] + index = 0 + length = len(body) + while index < length: + char = body[index] + if char != "\\": + out.append(char) + index += 1 + continue + if index + 1 >= length: + return None + marker = body[index + 1] + if marker in _GO_ESCAPES: + out.append(_GO_ESCAPES[marker]) + index += 2 + continue + if marker in _OCTAL_DIGITS: + digits = body[index + 1 : index + 4] + if len(digits) != 3 or any(char not in _OCTAL_DIGITS for char in digits): + return None + value = int(digits, 8) + if value > 255: + return None + out.append(chr(value)) + index += 4 + continue + widths = {"x": 2, "u": 4, "U": 8} + if marker in widths: + value = _hex_value(body, index + 2, widths[marker]) + if value is None or value > 0x10FFFF: + return None + out.append(chr(value)) + index += 2 + widths[marker] + continue + # Every other escape is a Go compile error, so the file this reader is + # looking at is not the file that built the server. + return None + return "".join(out) + + +class _Masker: + """Shared bookkeeping for the two language maskers.""" + + def __init__(self, text: str, language: str) -> None: + self.text = text + self.language = language + self.out: list[str] = list(text) + self.literals: dict[int, tuple[str | None, int]] = {} + self.anomalies: list[str] = [] + + def blank(self, start: int, end: int, fill: str) -> None: + end = min(end, len(self.out)) + if end <= start: + return + segment = self.text[start:end] + # Newlines survive so line numbers stay the file's own. Slice + # assignment rather than a per-character loop: the latter cost more + # than a second on a 1.4 MB module that registers nothing. + if "\n" in segment: + self.out[start:end] = ["\n" if char == "\n" else fill for char in segment] + else: + self.out[start:end] = fill * (end - start) + + def record(self, start: int, end: int, value: str | None) -> None: + self.blank(start, end, _STRING_FILL) + self.literals[start] = (value, end) + + def result(self) -> MaskedSource: + return MaskedSource( + text=self.text, + masked="".join(self.out), + literals=self.literals, + anomalies=tuple(self.anomalies), + ) + + +def _previous_significant(masked: list[str], index: int) -> tuple[str, int]: + while index >= 0 and masked[index].isspace(): + index -= 1 + return (masked[index], index) if index >= 0 else ("", -1) + + +def _preceding_word(masked: list[str], index: int) -> str: + """The identifier ending at or before ``index``, read from the mask. + + The mask, not the raw text: comments have been overwritten with spaces + there, so `if /*why*/ (ok) /re/` still finds `if`. Reading the raw text + found `/` — the tail of the comment — decided the slash was division, and + scanned the regex body as code, which reported a tool invented out of a + pattern. That is the one outcome masking exists to make impossible. + """ + + while index >= 0 and masked[index].isspace(): + index -= 1 + end = index + 1 + while index >= 0 and (masked[index].isalnum() or masked[index] in "_$"): + index -= 1 + return "".join(masked[index + 1 : end]) + + +def _mask_typescript(text: str) -> MaskedSource: + masker = _Masker(text, "typescript") + index = 0 + length = len(text) + while index < length: + found = _INTERESTING.search(text, index) + if found is None: + break + index = found.start() + char = text[index] + if char == "/" and index + 1 < length and text[index + 1] == "/": + end = text.find("\n", index) + end = length if end == -1 else end + masker.blank(index, end, _COMMENT_FILL) + index = end + continue + if char == "/" and index + 1 < length and text[index + 1] == "*": + end = text.find("*/", index + 2) + if end == -1: + masker.blank(index, length, _COMMENT_FILL) + masker.anomalies.append("unterminated_block_comment") + break + masker.blank(index, end + 2, _COMMENT_FILL) + index = end + 2 + continue + if char in {"'", '"'}: + index = _consume_quoted(masker, index, char, allow_newline=False) + continue + if char == "`": + index = _consume_template(masker, index) + continue + if char == "/" and _opens_regex(masker.out, index): + index = _consume_regex(masker, index) + continue + index += 1 + return masker.result() + + +def _opens_regex(out: list[str], index: int) -> bool: + previous, previous_index = _previous_significant(out, index - 1) + if previous == "" or previous in _REGEX_PRECEDING_CHARS: + return True + if previous == ")": + # A `)` is usually the end of a call or a parenthesised expression, and + # `foo(a) / 2` divides. But it is also the end of a control statement's + # condition, and there a regex validly *begins the body*: + # `if (ok) /\.registerTool("fake", handler)/.test(value);` is + # JavaScript, and reading its `/` as division scanned the pattern as + # code and reported a `fake` tool — a registration invented out of a + # regex body, which is the one thing this module's masking exists to + # make impossible. Which of the two it is, is decided by the keyword in + # front of the matching `(`. + opener = _matching_open(out, previous_index) + if opener is None: + return False + return _preceding_word(out, opener - 1) in _REGEX_PRECEDING_STATEMENTS + if previous.isalnum() or previous in "_$": + return _preceding_word(out, index - 1) in _REGEX_PRECEDING_WORDS + return False + + +def _matching_open(out: list[str], close_index: int) -> int | None: + depth = 0 + for index in range(close_index, -1, -1): + char = out[index] + if char == ")": + depth += 1 + elif char == "(": + depth -= 1 + if depth == 0: + return index + return None + + +def _past_escape(text: str, index: int, language: str) -> int: + """The index just past the escape whose backslash sits at ``index``. + + Two characters, except for a JavaScript line continuation spelled with + CRLF, which is three: the backslash and one *line terminator sequence*. + Stepping over two of them leaves the ``\n`` behind, and the scanner then + ends the string there — so the identical file lost its registration on a + Git-for-Windows checkout while resolving it on a Unix one. + + Go has no line continuation, and its scanner must keep treating a newline + as the end of an interpreted string, so this is TypeScript's rule only. + """ + + if language == "typescript" and text[index + 1 : index + 3] == "\r\n": + return index + 3 + return index + 2 + + +def _consume_quoted( + masker: _Masker, start: int, quote: str, *, allow_newline: bool +) -> int: + text = masker.text + length = len(text) + index = start + 1 + while index < length: + char = text[index] + if char == "\\": + index = _past_escape(text, index, masker.language) + continue + if char == quote: + masker.record( + start, index + 1, decode_literal(text[start + 1 : index], masker.language) + ) + return index + 1 + if char == "\n" and not allow_newline: + break + index += 1 + # Unterminated. Blank to the resync point but record no literal, and say so: + # past here this reader cannot tell code from content. + end = text.find("\n", start) + end = length if end == -1 or allow_newline else end + masker.blank(start, end, _STRING_FILL) + masker.anomalies.append("unterminated_string") + return max(end, start + 1) + + +def _consume_template(masker: _Masker, start: int) -> int: + """Consume a backtick template literal, tracking ``${…}`` substitutions.""" + + text = masker.text + length = len(text) + end, substituted = _template_end(text, masker.out, start) + if end is None: + masker.blank(start, length, _STRING_FILL) + masker.anomalies.append("unterminated_string") + return length + body = text[start + 1 : end - 1] + masker.record( + start, end, None if substituted else decode_literal(body, masker.language) + ) + return end + + +def _template_end( + text: str, out: list[str], start: int +) -> tuple[int | None, bool]: + """Where the template literal at ``start`` ends, and whether it substitutes. + + ``None`` when it never closes. The second value says whether the *outer* + template carries a ``${…}``, which is what makes its value non-constant. + + **A `${…}` holds code, so a brace inside a string, a comment, a regex or a + nested template is not a structural brace.** Counting them made + ``const msg = `Literal brace: ${"{"}`;`` leave the substitution open, and + from there the rest of the file was consumed as one unterminated template + — every registration after that line silently gone, and a workspace that + declares an MCP dependency reported as "not an agent project" over a brace + in a string (#485 review). + + Iterative, with one stack entry per open template, because a nested + template is reached through a substitution and recursion on attacker-shaped + input is a crash rather than a wrong answer. + """ + + length = len(text) + index = start + 1 + # One entry per open template: its `${…}` brace depth, 0 in template text. + depths: list[int] = [0] + substituted = False + while index < length and depths: + char = text[index] + if char == "\\": + index = _past_escape(text, index, "typescript") + continue + if depths[-1] == 0: + if char == "$" and text[index + 1 : index + 2] == "{": + substituted = substituted or len(depths) == 1 + depths[-1] = 1 + index += 2 + continue + if char == "`": + depths.pop() + index += 1 + continue + index += 1 + continue + if char in {"'", '"'}: + index = _skip_quoted(text, index) + continue + if char == "`": + depths.append(0) + index += 1 + continue + if char == "/" and text[index + 1 : index + 2] == "/": + line_end = text.find("\n", index) + line_end = length if line_end == -1 else line_end + # Blanked as it is walked, not merely stepped over: the regex + # heuristic below reads the mask to find the keyword in front of a + # slash, and a comment still spelled out there hides it. + out[index:line_end] = _COMMENT_FILL * (line_end - index) + index = line_end + continue + if char == "/" and text[index + 1 : index + 2] == "*": + close = text.find("*/", index + 2) + block_end = length if close == -1 else close + 2 + out[index:block_end] = [ + "\n" if character == "\n" else _COMMENT_FILL + for character in text[index:block_end] + ] + index = block_end + continue + if char == "/" and _opens_regex(out, index): + index = _skip_regex(text, index) + continue + if char == "{": + depths[-1] += 1 + elif char == "}": + depths[-1] -= 1 + index += 1 + return (index if not depths else None), substituted + + +def _skip_quoted(text: str, start: int) -> int: + """Index just past a quoted string this reader only needs to walk over.""" + + quote = text[start] + length = len(text) + index = start + 1 + while index < length: + char = text[index] + if char == "\\": + index = _past_escape(text, index, "typescript") + continue + if char == quote: + return index + 1 + if char == "\n": + # Unterminated on its line. Resync there rather than swallowing the + # rest of the substitution. + return index + index += 1 + return length + + +def _skip_regex(text: str, start: int) -> int: + """Index just past a regex literal, or one past the slash if it is not one.""" + + length = len(text) + index = start + 1 + in_class = False + while index < length: + char = text[index] + if char == "\\": + index += 2 + continue + if char == "\n": + return start + 1 + if char == "[": + in_class = True + elif char == "]": + in_class = False + elif char == "/" and not in_class: + return index + 1 + index += 1 + return start + 1 + + +def _consume_regex(masker: _Masker, start: int) -> int: + text = masker.text + length = len(text) + index = start + 1 + in_class = False + while index < length: + char = text[index] + if char == "\\": + index += 2 + continue + if char == "\n": + # Not a regex after all — a lone `/` on a line. Leave it as code. + return start + 1 + if char == "[": + in_class = True + elif char == "]": + in_class = False + elif char == "/" and not in_class: + masker.blank(start, index + 1, _COMMENT_FILL) + return index + 1 + index += 1 + return start + 1 + + +def _mask_go(text: str) -> MaskedSource: + masker = _Masker(text, "go") + index = 0 + length = len(text) + while index < length: + found = _INTERESTING.search(text, index) + if found is None: + break + index = found.start() + char = text[index] + if char == "/" and index + 1 < length and text[index + 1] == "/": + end = text.find("\n", index) + end = length if end == -1 else end + masker.blank(index, end, _COMMENT_FILL) + index = end + continue + if char == "/" and index + 1 < length and text[index + 1] == "*": + end = text.find("*/", index + 2) + if end == -1: + masker.blank(index, length, _COMMENT_FILL) + masker.anomalies.append("unterminated_block_comment") + break + masker.blank(index, end + 2, _COMMENT_FILL) + index = end + 2 + continue + if char == '"': + index = _consume_quoted(masker, index, '"', allow_newline=False) + continue + if char == "'": + index = _consume_quoted(masker, index, "'", allow_newline=False) + continue + if char == "`": + end = text.find("`", index + 1) + if end == -1: + masker.blank(index, length, _STRING_FILL) + masker.anomalies.append("unterminated_string") + break + masker.record(index, end + 1, text[index + 1 : end]) + index = end + 1 + continue + index += 1 + return masker.result() + + +# Idiom matchers. + +_TS_MODIFIERS = r"(?:(?:public|private|protected|readonly|override|declare|abstract)\s+)*" +_TS_STATIC_TOOL_NAME_RE = re.compile( + rf"(? int | None: + depth = 0 + for index in range(open_index, len(masked)): + char = masked[index] + if char == opener: + depth += 1 + elif char == closer: + depth -= 1 + if depth == 0: + return index + 1 + return None + + +def _brace_pairs(masked: str) -> list[tuple[int, int]]: + stack: list[int] = [] + pairs: list[tuple[int, int]] = [] + for match in re.finditer(r"[{}]", masked): + if match.group() == "{": + stack.append(match.start()) + elif stack: + pairs.append((stack.pop(), match.start() + 1)) + return pairs + + +def _enclosing_block(pairs: list[tuple[int, int]], index: int) -> tuple[int, int] | None: + """The innermost ``{…}`` containing ``index``, as ``(open, close_exclusive)``.""" + best: tuple[int, int] | None = None + for start, end in pairs: + if start <= index < end and (best is None or start > best[0]): + best = (start, end) + return best + + +def _resolve_name(value: str | None, found: bool) -> tuple[str | None, str | None]: + if not found or value is None: + return None, "name_not_literal" + if not TOOL_NAME_RE.match(value): + return None, "implausible_tool_name" + return value, None + + +#: Characters that continue an expression rather than beginning a statement. +#: Consulted only *after* a line break, and only for a character the caller's +#: own terminators do not claim: Go ends a struct field with `,` on the next +#: line, and that comma ends the value rather than continuing it. +_EXPRESSION_CONTINUATION = frozenset("+-*/%&|^<>=!?.,([") + + +def _literal_is_whole_value( + source: MaskedSource, end: int, terminators: str +) -> bool: + """Whether the literal ending at ``end`` is the entire value, not part of one. + + ``static toolName = "backup" + SUFFIX`` resolves to a literal this reader + can see, and reading it as the tool name would publish ``backup`` for a + tool the server registers under some other name — a fail-open of exactly + the shape #393 catalogues, where the proof rests on a spelling. The literal + counts only when the expression ends there: at one of ``terminators``, at + the end of input, or at a line break (JavaScript inserts the semicolon). + """ + + masked = source.masked + length = len(masked) + index = end + while index < length and masked[index] in " \t\r": + index += 1 + if index >= length: + return True + if masked[index] in terminators: + return True + if masked[index] != "\n": + return False + # A line break ends the statement only when what follows cannot continue + # the expression. `static toolName = "safe"` followed by `+ "_delete"` on + # the next line is one value spelled across two lines, and accepting the + # first literal publishes `safe` for a tool the server registers as + # `safe_delete` — a name nobody serves, at `medium` confidence, which is + # worse than the omission refusing it produces. Comments are already + # spaces in the mask, so skipping whitespace skips them too. + while index < length and masked[index].isspace(): + index += 1 + if index >= length: + return True + following = masked[index] + return following in terminators or following not in _EXPRESSION_CONTINUATION + + +def _call_sites( + source: MaskedSource, pattern: re.Pattern[str], idiom: str +) -> list[RegistrationSite]: + """Sites for a ``Name(, …)`` idiom.""" + sites: list[RegistrationSite] = [] + for match in pattern.finditer(source.masked): + open_paren = source.masked.rfind("(", match.start(), match.end()) + if open_paren == -1: + continue + close = _matching_close(source.masked, open_paren, "(", ")") + span = (match.start(), close if close is not None else match.end()) + found, value, end = source.literal_at(match.end()) + name, unresolved = _resolve_name(value, found) + if name is not None: + # A registration passes the name *and* what to do with it, so the + # first argument is followed by a comma. `)` means a one-argument + # call — `map.tool("issues")` is a lookup, and reading it as a + # registration is how an accessor becomes a phantom tool. Anything + # else (`+`) means the name is not this literal. + after = source.skip_space(end) + following = source.masked[after] if after < len(source.masked) else "" + if following == ")": + continue + if following != ",": + name, unresolved = None, "name_not_literal" + if name is None and ( + close is None or not _has_second_argument(source.masked, open_paren, close) + ): + # An unresolved site needs the same second argument before it is + # reported: it is what keeps `map.tool(key)` out of the ledger. + continue + line, column = source.line_column(match.start()) + sites.append( + RegistrationSite( + idiom=idiom, + name=name, + line=line, + column=column, + span=span, + unresolved_reason=unresolved, + ) + ) + return sites + + +def _has_second_argument(masked: str, open_paren: int, close: int) -> bool: + depth = 0 + for index in range(open_paren, close): + char = masked[index] + if char in "([{": + depth += 1 + elif char in ")]}": + depth -= 1 + elif char == "," and depth == 1: + return True + return False + + +def _ts_static_tool_name_sites(source: MaskedSource) -> list[RegistrationSite]: + sites: list[RegistrationSite] = [] + pairs = _brace_pairs(source.masked) + for match in _TS_STATIC_TOOL_NAME_RE.finditer(source.masked): + found, value, end = source.literal_at(match.end()) + name, unresolved = _resolve_name(value, found) + if name is not None and not _literal_is_whole_value(source, end, ";}"): + name, unresolved = None, "name_not_literal" + line, column = source.line_column(match.start()) + block = _enclosing_block(pairs, match.start()) + operation_type: str | None = None + description: str | None = None + if block is not None and name is not None: + operation_type = _first_literal_in(source, _TS_STATIC_OPERATION_TYPE_RE, block) + description = _first_literal_in(source, _TS_DESCRIPTION_RE, block) + # The construct, never the enclosing class body. `block` is the scope + # the sibling literals are looked up in; using it as the span made the + # containment rule read *any* registration written inside the class as + # "the same registration", so a class whose `toolName` is built at + # runtime lost its omission the moment it also called `.registerTool(`. + sites.append( + RegistrationSite( + idiom="ts_static_tool_name", + name=name, + line=line, + column=column, + span=(match.start(), max(end, match.end())), + description=description, + operation_type=operation_type, + unresolved_reason=unresolved, + ) + ) + return sites + + +def _first_literal_in( + source: MaskedSource, pattern: re.Pattern[str], block: tuple[int, int] +) -> str | None: + start, end = block + for match in pattern.finditer(source.masked, start, end): + found, value, literal_end = source.literal_at(match.end()) + if found and value and _literal_is_whole_value(source, literal_end, ";}"): + return value + return None + + +def _go_tool_struct_sites(source: MaskedSource) -> list[RegistrationSite]: + sites: list[RegistrationSite] = [] + for match in _GO_TOOL_STRUCT_RE.finditer(source.masked): + open_brace = source.masked.index("{", match.start()) + close = _matching_close(source.masked, open_brace, "{", "}") + if close is None: + continue + if _has_keyed_field(source.masked, open_brace, close): + sites.extend(_go_tool_struct_site(source, match.start(), open_brace, close)) + continue + # No keyed field at this literal's own level, so it is a composite of + # elements — `[]mcp.Tool{{Name: "a"}, {Name: "b"}}`. Reading only the + # outer brace would find the first element's `Name:` two levels down, + # reject it as nested, and report nothing at all. + for child_open, child_close in _child_braces(source.masked, open_brace, close): + sites.extend(_go_tool_struct_site(source, child_open, child_open, child_close)) + return sites + + +def _go_tool_struct_site( + source: MaskedSource, start: int, open_brace: int, close: int +) -> list[RegistrationSite]: + field = _GO_STRUCT_NAME_FIELD_RE.search(source.masked, open_brace + 1, close) + # Only the literal's own `Name:` field, never one belonging to something + # nested inside it: `mcp.Tool{Annotations: &mcp.ToolAnnotations{Name: …}}` + # names the annotation, not the tool. + while field is not None and _brace_depth(source.masked, open_brace, field.start()) != 1: + field = _GO_STRUCT_NAME_FIELD_RE.search(source.masked, field.end(), close) + if field is None: + return [] + found, value, literal_end = source.literal_at(field.end()) + name, unresolved = _resolve_name(value, found) + if name is not None and not _literal_is_whole_value(source, literal_end, ",}"): + name, unresolved = None, "name_not_literal" + line, column = source.line_column(start) + return [ + RegistrationSite( + idiom="go_tool_struct", + name=name, + line=line, + column=column, + span=(start, close), + description=_go_struct_description(source, open_brace, close), + unresolved_reason=unresolved, + ) + ] + + +def _has_keyed_field(masked: str, open_brace: int, close: int) -> bool: + """Whether the literal names fields at its own level (a struct, not a list).""" + for match in _GO_KEYED_FIELD_RE.finditer(masked, open_brace + 1, close): + if _brace_depth(masked, open_brace, match.start()) == 1: + return True + return False + + +def _child_braces(masked: str, open_brace: int, close: int) -> list[tuple[int, int]]: + children: list[tuple[int, int]] = [] + index = open_brace + 1 + while index < close - 1: + if masked[index] == "{": + child_close = _matching_close(masked, index, "{", "}") + if child_close is None or child_close > close: + break + children.append((index, child_close)) + index = child_close + continue + index += 1 + return children + + +def _go_struct_description(source: MaskedSource, open_brace: int, close: int) -> str | None: + for match in _GO_STRUCT_DESCRIPTION_FIELD_RE.finditer( + source.masked, open_brace + 1, close + ): + if _brace_depth(source.masked, open_brace, match.start()) != 1: + continue + found, value, literal_end = source.literal_at(match.end()) + if found and value and _literal_is_whole_value(source, literal_end, ",}"): + return value + return None + + +def _brace_depth(masked: str, open_brace: int, index: int) -> int: + depth = 0 + for position in range(open_brace, index): + char = masked[position] + if char == "{": + depth += 1 + elif char == "}": + depth -= 1 + return depth + + +def scan_source(text: str, language: str) -> SourceScanResult: + """Find every registration site in one file. + + An unresolved site is dropped when a resolved one sits inside it. The + wrapper shape is real and common — ``NewTool(metadata, mcp.Tool{Name: + "issue_read"}, …)`` is 132 of them in ``github/github-mcp-server`` — and + reporting the wrapper's non-literal first argument as an unenumerated tool + would fill the ledger with omissions for tools the very same call names one + argument later. + """ + if PREFILTER_TOKEN not in text.lower(): + return SourceScanResult() + source = mask_source(text, language) + sites: list[RegistrationSite] = [] + if language == "typescript": + sites.extend(_ts_static_tool_name_sites(source)) + sites.extend(_call_sites(source, _TS_REGISTER_TOOL_RE, "ts_sdk_register_tool")) + else: + sites.extend(_call_sites(source, _GO_MUST_TOOL_RE, "go_must_tool")) + sites.extend(_call_sites(source, _GO_NEW_TOOL_RE, "go_new_tool")) + sites.extend(_go_tool_struct_sites(source)) + + kept = [ + site + for site in sites + if site.name is not None or not _contains_another_site(site, sites) + ] + kept.sort(key=lambda site: (site.line, site.column, site.idiom)) + return SourceScanResult(sites=tuple(kept), anomalies=source.anomalies) + + +def _contains_another_site(site: RegistrationSite, sites: list[RegistrationSite]) -> bool: + """Whether a nested site describes the same registration as ``site``. + + Sound only because every ``span`` is the *construct* that registers. A span + standing for a lookup *scope* would make any registration written inside + that scope suppress the site, which is a different relationship entirely. + """ + start, end = site.span + return any( + start < other.span[0] and other.span[1] <= end + for other in sites + if other is not site + ) + + +# --- MCP source discovery (mirror of cli/discovery/mcp_source.py) ----------- +# +# Every function below is byte-for-byte the CLI's, with two deliberate +# exceptions a reviewer diffing the two files should expect: +# +# `_mcp_export_tool_names` — the CLI probes an export by calling the real +# `load_mcp_tools`, which is a pydantic-backed adapter. Here it is the same +# accept rule read with `json`, which is also what `_probe_mcp` above already +# mirrors for the same file. +# +# `_read_mcp_source_text` — the CLI's read is `inputs.common.load_text_file` +# (a regular file, at most 10 MB, decoded strict). It is factored out here so +# the contract is visible in one place: discovery that decoded leniently was +# a shipped defect, and a port that shared the path predicate but not the +# read would reintroduce it. + +#: How many source files discovery reads before it stops. Truncation is +#: reported, never silent. +DEFAULT_MAX_SOURCE_FILES = 1500 + +#: How many tool names the evidence names before it summarises. The line is +#: read by a human deciding whether to adopt, and 110 names is not evidence. +_EVIDENCE_NAME_LIMIT = 5 + +#: Dependency sections of a ``package.json`` that count. ``devDependencies`` is +#: included on purpose: the question is "was this repository written against +#: MCP", not "does it ship the SDK at runtime". +_PACKAGE_JSON_DEPENDENCY_KEYS = ( + "dependencies", + "devDependencies", + "peerDependencies", + "optionalDependencies", +) + + +@dataclass +class _FrameworkEvidence: + """Declared MCP dependencies, per language.""" + + languages: set[str] = field(default_factory=set) + reasons: list[str] = field(default_factory=list) + + +@dataclass(frozen=True) +class McpSourceDiscovery: + """What discovery concluded about the workspace's own registration sites.""" + + path: str | None = None + languages: tuple[str, ...] = () + tool_names: tuple[str, ...] = () + unresolved_count: int = 0 + evidence: tuple[str, ...] = () + #: The declared-dependency reasons behind the language gate, on their own. + #: Scoring reads this rather than indexing into ``evidence``: the rendered + #: lines are ordered for a human and gain conditional entries at the end. + framework_evidence: tuple[str, ...] = () + candidate_files: tuple[str, ...] = () + excluded: tuple[dict[str, str], ...] = () + truncated: bool = False + + @property + def detected(self) -> bool: + return self.path is not None + + +def _discover_mcp_server_source( + workspace: Path, + files: list[Path], + exported_source_paths: list[str], + max_source_files: int = DEFAULT_MAX_SOURCE_FILES, +) -> McpSourceDiscovery: + """Decide whether this workspace registers MCP tools in its own source. + + Two facts have to hold together, and the pairing is the whole design. A + **declared MCP dependency** turns a spelling into provenance — a repository + that declares none is not an MCP server because a class of its own happens + to spell a field ``toolName``. A **resolved registration** is the other + half: the dependency alone says the repository uses MCP, which every client + does too. + + ``exported_source_paths`` are the workspace-relative paths of MCP exports + already accepted as suggestions. An export that names every registration + withholds this route: it is the server's own published contract, carries + the input schemas this route does not read, and is read at high confidence + against medium. + """ + workspace = workspace.resolve() + framework = _mcp_framework_evidence(workspace, files) + if not framework.languages: + return McpSourceDiscovery() + + # Paired with the workspace-relative path once, here: a file that is not + # under the workspace at all (a symlink out of the tree) has no relative + # form, and inventing one from its basename would put it in the wrong + # directory for both the skip rules and the route's common ancestor. + scannable = [ + pair + for pair in ((path, _mcp_relative(path, workspace)) for path in files) + if pair[1] is not None + and _mcp_language_in_scope(pair[1], framework.languages) + ] + # Sorted before the cap, so which files are read is a property of the + # workspace and not of the walk order — and capped where the flag is set, + # rather than beside it. + scannable.sort(key=lambda pair: pair[1]) + truncated = len(scannable) > max_source_files + names: set[str] = set() + languages: set[str] = set() + unresolved_by_file: dict[str, int] = {} + candidate_files: list[str] = [] + for path, relative in scannable[:max_source_files]: + language = language_for_path(path) + if language is None: # pragma: no cover - filtered above + continue + try: + if path.stat().st_size > MAX_SOURCE_FILE_BYTES: + continue + except OSError: + continue + text = _read_mcp_source_text(path) + if text is None: + continue + result = scan_source(text, language) + resolved = [site.name for site in result.sites if site.name is not None] + opaque = sum(1 for site in result.sites if site.name is None) + if opaque: + unresolved_by_file[relative] = opaque + if not resolved: + continue + languages.add(language) + names.update(resolved) + candidate_files.append(relative) + + if not names: + return McpSourceDiscovery( + unresolved_count=sum(unresolved_by_file.values()), truncated=truncated + ) + + root = _mcp_common_directory(candidate_files) + # Counted over the directory the route actually points at, so the number + # this publishes is the number the adapter will report once the route is + # configured. + unresolved = sum( + count + for relative, count in unresolved_by_file.items() + if root == "." or PurePosixPath(relative).is_relative_to(root) + ) + evidence = _mcp_evidence_lines( + framework, languages, names, root, truncated, unresolved + ) + covering_export, uncovered = _mcp_covering_export( + workspace, exported_source_paths, names + ) + if covering_export is not None: + return McpSourceDiscovery( + unresolved_count=unresolved, + truncated=truncated, + excluded=( + { + "type": MCP_SOURCE_TYPE, + "path": root, + "reason": ( + f"An MCP tool export ({covering_export}) already names " + f"every one of these {len(names)} registrations, and an " + "export is read at high confidence with its input " + "schemas; reading them in source would restate it at " + "medium." + ), + }, + ), + ) + if uncovered: + # An export exists and does not account for the whole surface. It used + # to withhold this route anyway, which in a workspace holding two + # servers meant an export for one erased every source-only registration + # of the other. Both routes are suggested instead, and the overlap is + # named. + sample = ", ".join(sorted(uncovered)[:_EVIDENCE_NAME_LIMIT]) + if len(uncovered) > _EVIDENCE_NAME_LIMIT: + sample += ", …" + evidence = ( + *evidence, + f"An MCP tool export is also present and does not name " + f"{len(uncovered)} of these registrations ({sample}); both routes " + "are suggested, and a reviewed tool_identity binding is what joins " + "the two surfaces", + ) + + return McpSourceDiscovery( + path=root, + languages=tuple(sorted(languages)), + tool_names=tuple(sorted(names)), + unresolved_count=unresolved, + evidence=evidence, + framework_evidence=tuple(sorted(framework.reasons)), + candidate_files=tuple(sorted(candidate_files)), + truncated=truncated, + ) + + +def _read_mcp_source_text(path: Path) -> str | None: + """The adapter's own read, not a lenient copy of it. + + Decoding with ``errors="replace"`` let ``detect`` resolve a registration + out of a file the scan-time loader then refuses as ``unreadable_file``, so + the route promised more tools than it could enumerate. Sharing the path + predicate is not enough — the read has to be shared too. + """ + try: + if not path.is_file(): + return None + data = path.read_bytes() + except OSError: + return None + try: + return data.decode("utf-8") + except UnicodeDecodeError: + return None + + +def _mcp_framework_evidence(workspace: Path, files: list[Path]) -> _FrameworkEvidence: + evidence = _FrameworkEvidence() + for path in files: + name = path.name + if name not in {"package.json", "go.mod"}: + continue + relative = _mcp_relative(path, workspace) + if relative is None: + continue + # The same skip set the source walk uses, not a narrower one of its + # own: two lists would let a directory be skipped for registrations + # while still granting the language gate that admits them. + parts = PurePosixPath(relative).parts + if any(part in SKIP_DIRECTORY_NAMES for part in parts): + continue + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + if name == "go.mod": + lowered = text.lower() + for module in GO_FRAMEWORK_MODULES: + if module in lowered: + evidence.languages.add("go") + evidence.reasons.append(f"{relative} requires {module}") + break + continue + package = _mcp_package_dependencies(text) + for dependency in sorted(package): + if any( + dependency.lower().startswith(token) + for token in TYPESCRIPT_FRAMEWORK_PACKAGES + ): + evidence.languages.add("typescript") + evidence.reasons.append(f"{relative} depends on {dependency}") + break + return evidence + + +def _mcp_package_dependencies(text: str) -> set[str]: + try: + data = json.loads(text) + except ValueError: + return set() + if not isinstance(data, dict): + return set() + names: set[str] = set() + for key in _PACKAGE_JSON_DEPENDENCY_KEYS: + section = data.get(key) + if isinstance(section, dict): + names.update(str(name) for name in section) + return names + + +def _mcp_language_in_scope(relative: str, languages: set[str]) -> bool: + language = language_for_path(relative) + if language is None or language not in languages: + return False + return is_scannable_path(relative) + + +def _mcp_relative(path: Path, workspace: Path) -> str | None: + """The CLI's own relative-path rule for this route, not this script's `_rel`. + + ``_rel`` prefers the *logical* name and falls back to the path itself, which + is right for the inventory it serves. Here the answer decides the route's + common ancestor and which skip rules apply, and the CLI resolves — so this + resolves too. Two spellings of one path is how the two detectors would + disagree about which directory a manifest should point at. + """ + try: + return path.resolve().relative_to(workspace).as_posix() + except (OSError, ValueError): + return None + + +def _mcp_common_directory(relative_files: list[str]) -> str: + """The deepest directory containing every file that registered a tool. + + Not a scope decision — #363 settled that a deepest-common-ancestor is the + wrong answer for *which project a manifest describes*. This is the narrower + question of which subtree the adapter walks, where widening only costs a + longer walk. + """ + parts_list = [PurePosixPath(name).parent.parts for name in relative_files] + if not parts_list: + return "." + common = parts_list[0] + for parts in parts_list[1:]: + limit = min(len(common), len(parts)) + index = 0 + while index < limit and common[index] == parts[index]: + index += 1 + common = common[:index] + return PurePosixPath(*common).as_posix() if common else "." + + +def _mcp_covering_export( + workspace: Path, exported_source_paths: list[str], names: set[str] +) -> tuple[str | None, set[str]]: + """The export that names *every* registration, and what no export names. + + Location is not the test, and neither is mere existence: "any export + anywhere wins" let an export for one server erase another server's + source-only registrations, and let a partial export erase the remainder of + a single one. Only containment makes withholding lossless. + """ + candidates = sorted(exported_source_paths) + if not candidates: + # No export at all — the common case for a source-only server. Without + # this the caller would report "an MCP tool export is also present and + # does not name them" about a file that does not exist. + return None, set() + covered: set[str] = set() + for candidate in candidates: + exported = _mcp_export_tool_names(workspace, candidate) + if exported is None: + continue + covered |= exported + if names <= covered: + return candidate, set() + return None, names - covered + + +def _mcp_export_tool_names(workspace: Path, relative: str) -> set[str] | None: + """Tool names an accepted MCP export publishes. Mirrors ``load_mcp_tools``. + + ``None`` when the export declines to name them — a wildcard export claims a + surface without enumerating it, so it can never be shown to contain + anything, and the source route is the more informative of the two. + """ + export = workspace / relative + try: + # Bounded like the loader this mirrors: `load_mcp_tools` refuses a file + # over `MAX_INPUT_FILE_BYTES` before parsing it, so an export past the + # bound names nothing on either side — and this reader is reached with + # a path chosen by the workspace, in a script that is curled onto an + # unknown repository. + if export.stat().st_size > MAX_STRUCTURED_FILE_BYTES: + return None + data = json.loads(export.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, ValueError): + return None + if isinstance(data, list): + raw_tools: Any = data + elif isinstance(data, dict): + raw_tools = data.get("tools") + if data.get("wildcard") is True or raw_tools == "*": + # Wildcard exposure, and the wildcard-plus-array contradiction the + # loader refuses outright, both reach the caller the same way: + # there are no names here to contain anything. + # + # No fixture can distinguish this branch from the fall-through + # below, and that is a fact about the *caller*, not a gap: every + # wildcard shape `_probe_mcp` accepts also has no usable `tools` + # array, and the one that does — wildcard plus a populated array — + # is refused at the probe and never reaches this function. The + # branch stays because it mirrors `load_mcp_tools`, and resting on + # the probe to make it redundant would couple this reader to a + # gate that is not its own. + return None + else: + return None + if not isinstance(raw_tools, list): + return None + return { + str(entry["name"]) + for entry in raw_tools + if isinstance(entry, dict) and entry.get("name") + } + + +def _mcp_evidence_lines( + framework: _FrameworkEvidence, + languages: set[str], + names: set[str], + root: str, + truncated: bool, + unresolved: int, +) -> tuple[str, ...]: + sample = sorted(names)[:_EVIDENCE_NAME_LIMIT] + shown = ", ".join(sample) + if len(names) > len(sample): + shown += ", …" + lines = [ + f"MCP tool registrations in {'/'.join(sorted(languages))} source under " + f"{root}/: {len(names)} tool name(s) — {shown}", + ] + lines.extend(sorted(framework.reasons)[:_EVIDENCE_NAME_LIMIT]) + if unresolved: + # Named here because this is what a human reads when deciding whether + # to adopt, and "61 tools" without "and 3 more this reader cannot name" + # is the over-claim the whole input is built to avoid. + lines.append( + f"{unresolved} registration(s) name themselves at runtime and are " + "not enumerated" + ) + if truncated: + lines.append( + "Discovery stopped at the source-file cap, so this count is a " + "lower bound." + ) + return tuple(lines) + + +def _score_mcp_server_source( + discovery: McpSourceDiscovery, scores: dict[str, dict[str, Any]] +) -> None: + """Score the workspace's own MCP registration sites (mirror of signals.py). + + The registration evidence reaches the detection threshold on its own, + because the fact behind it is already a conjunction: a declared MCP + dependency *and* a tool name resolved at a registration site. The + dependency then adds the same point a dependency adds for every other + framework. + + The candidate file is the **route directory**, not the registration files + under it: an MCP server is one thing however many packages its tools are + spread across, and contributing each file made `mongodb-js/mongodb-mcp-server` + look like six separate projects. + """ + if not discovery.detected or discovery.path is None: + return + _add(scores, MCP_SOURCE_TYPE, 2.0, "strong", discovery.evidence[0]) + # The declared dependency is identified by *value* — the line is one of the + # discovery result's `framework_evidence` entries. Awarding it to + # `evidence[1]` read the point off a list position instead, in a list that + # is ordered for a human and gains conditional entries at the end. + reasons = set(discovery.framework_evidence) + awarded = False + for line in discovery.evidence[1:]: + dependency = not awarded and line in reasons + awarded = awarded or dependency + _add( + scores, + MCP_SOURCE_TYPE, + 1.0 if dependency else 0.0, + "medium" if dependency else "supporting", + line, + ) + # `_add`'s candidate argument would attribute the directory to whichever + # evidence line happened to be last; the route directory belongs to the + # detection, not to a line of prose about it. + if discovery.path not in scores[MCP_SOURCE_TYPE]["candidate_files"]: + scores[MCP_SOURCE_TYPE]["candidate_files"].append(discovery.path) + + def _name(node: ast.AST) -> str | None: if isinstance(node, ast.Call): return _name(node.func) @@ -1825,37 +3457,10 @@ def detect(workspace: Path) -> dict[str, Any]: present_dirs = [ conventional_locations[d] for d in CONVENTIONAL_DIRS if d in conventional_locations ] - for fw in FRAMEWORKS: + for fw in CONVENTIONAL_DIR_FRAMEWORKS: for d in present_dirs: _add(scores, fw, 0.5, "weak", f"conventional dir: {d}/") - detections: list[dict[str, Any]] = [ - { - "type": fw, - "score": round(st["score"], 2), - "confidence": _confidence(st["score"]), - "evidence": st["evidence"], - "candidate_files": st["candidate_files"], - } - for fw, st in scores.items() - if st["score"] >= 2.0 and st["has_strong"] - ] - detections.sort(key=lambda d: (-d["score"], d["type"])) - - project_names: list[dict[str, str]] = [] - pyproject = workspace / "pyproject.toml" - if pyproject.is_file(): - try: - text = pyproject.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - text = "" - m = PYPROJECT_NAME_RE.search(text) - if m: - project_names.append({"value": m.group(1).strip(), "source": "pyproject"}) - project_names.append({"value": workspace.name, "source": "workspace_dir"}) - - name_candidates = _rank_agent_names(py_facts, workspace, project_names) - # Glob candidates, then keep only the ones the input adapters accept — # a glob hit (e.g. an mcpServers-style host config matching *mcp*.json) # that fails the probe would make the next init->scan step fail, so it @@ -1889,6 +3494,46 @@ def detect(workspace: Path) -> dict[str, Any]: failures.append({"type": kind, "path": p, "reason": reason}) excluded = [e for e in failures if e["path"] not in suggested_paths] + # The artifact probe runs before the detection loop because the source + # route below is scored from its result: an MCP export is the better route + # to the same server, so this one stands down wherever one exists (#431). + mcp_source = _discover_mcp_server_source( + workspace, + files, + [s["path"] for s in suggested if s["type"] == "mcp"], + ) + _score_mcp_server_source(mcp_source, scores) + excluded.extend(mcp_source.excluded) + if mcp_source.path is not None: + suggested.append({"type": MCP_SOURCE_TYPE, "path": mcp_source.path}) + + detections: list[dict[str, Any]] = [ + { + "type": fw, + "score": round(st["score"], 2), + "confidence": _confidence(st["score"]), + "evidence": st["evidence"], + "candidate_files": st["candidate_files"], + } + for fw, st in scores.items() + if st["score"] >= 2.0 and st["has_strong"] + ] + detections.sort(key=lambda d: (-d["score"], d["type"])) + + project_names: list[dict[str, str]] = [] + pyproject = workspace / "pyproject.toml" + if pyproject.is_file(): + try: + text = pyproject.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + text = "" + m = PYPROJECT_NAME_RE.search(text) + if m: + project_names.append({"value": m.group(1).strip(), "source": "pyproject"}) + project_names.append({"value": workspace.name, "source": "workspace_dir"}) + + name_candidates = _rank_agent_names(py_facts, workspace, project_names) + marketplace_paths = [ path for path in files