From 08a14a642be25fb9076a953240ea9f0ac7e02fe2 Mon Sep 17 00:00:00 2001 From: JP Date: Fri, 7 Aug 2026 15:34:37 +0100 Subject: [PATCH] feat: fail the gate when a trigger is recorded but unroutable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moving routing keywords out of `triggers` and into `description` left the metadata copy as a second source of truth with nothing keeping the two in step. A trigger that lives only in metadata is unroutable — no conformant client reads metadata, they match on `description` alone — so it would rot silently and give a false impression of coverage. validate_skills.py and the smoke tests now assert every metadata trigger appears in the description, making the metadata index a derived view rather than a competitor. Current main satisfies this already; the check is a regression guard, verified to fail on an injected unroutable trigger. Tests 195 -> 212. skills/mcp-servers/SKILL.md gains the section explaining what the portable mcp.json ships versus what the reader opts into, and why Toolbox is excluded. Cherry-picked from the parallel branch on PR #4. --- scripts/validate_skills.py | 23 ++++++++++++ skills/mcp-servers/SKILL.md | 37 ++++++++++++++++++- .../skill-smoke-tests/test_skill_contract.py | 13 +++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/scripts/validate_skills.py b/scripts/validate_skills.py index 241b9f6..6595eba 100644 --- a/scripts/validate_skills.py +++ b/scripts/validate_skills.py @@ -91,6 +91,27 @@ def check_metadata(metadata) -> list[str]: return errors +def check_triggers_routable(metadata: dict, description: str) -> list[str]: + """Every trigger must appear in the description, because that is what routes. + + No conformant client reads metadata — it matches on `description` alone. A + trigger recorded only in metadata is therefore unroutable, and the two + copies silently rot apart. This keeps the metadata index a derived view of + the description rather than a competing source of truth. + """ + raw = metadata.get(f"{NS}triggers") if isinstance(metadata, dict) else None + if not isinstance(raw, str): + return [] + lowered = description.lower() + unroutable = [t.strip() for t in raw.split(",") if t.strip() and t.strip().lower() not in lowered] + if unroutable: + return [ + f"trigger(s) {unroutable} appear in metadata but not in 'description' — " + "they are unroutable; add them to the description or drop them" + ] + return [] + + def validate_skill(skill_path: Path) -> list[str]: content = skill_path.read_text() @@ -141,6 +162,8 @@ def validate_skill(skill_path: Path) -> list[str]: errors.append("Missing 'metadata' (carries this plugin's skill contract)") else: errors.extend(check_metadata(fm["metadata"])) + if isinstance(fm.get("description"), str): + errors.extend(check_triggers_routable(fm["metadata"], fm["description"])) if not (skill_path.parent / "references").is_dir(): errors.append("Missing references/ directory") diff --git a/skills/mcp-servers/SKILL.md b/skills/mcp-servers/SKILL.md index 7dadd77..bcbd73f 100644 --- a/skills/mcp-servers/SKILL.md +++ b/skills/mcp-servers/SKILL.md @@ -26,6 +26,34 @@ gcloud auth activate-service-account --key-file=/path/to/sa-key.json gcloud auth application-default login --impersonate-service-account=SA@PROJECT.iam.gserviceaccount.com ``` +## What the plugin ships vs what you opt into + +The portable `mcp.json` (Agent Plugins 1.0.0) declares **one** server — `gcloud` +— because that is the only one that starts with no user-supplied configuration: + +```json +{ + "$schema": "https://agent-plugins.org/schemas/1.0.0/mcp.schema.json", + "mcpServers": { + "gcloud": { "type": "stdio", "command": "npx", "args": ["-y", "@google-cloud/gcloud-mcp"] } + } +} +``` + +Two consequences worth knowing: + +- **No `env` block.** Agent Plugins expands only `${PLUGIN_ROOT}` and + `${PLUGIN_DATA}`. A `${GCP_PROJECT_ID}` placeholder would be passed through + *literally* and mis-set your project. Export it in your own shell instead — + the client passes the ambient environment through: + ```bash + export CLOUDSDK_CORE_PROJECT=your-project-id + gcloud auth application-default login + ``` +- **The capability table below lists logical GCP capabilities**, not entries in + `mcp.json`. Most are reachable through the `gcloud` server; anything needing + its own config file is an opt-in you add to your client's own MCP settings. + ## Google-Managed MCP Servers (v0.1) | Server | Capability | Required Role | @@ -39,10 +67,17 @@ gcloud auth application-default login --impersonate-service-account=SA@PROJECT.i | `google-logging` | Cloud Logging query/ingest | `roles/logging.viewer` | | `google-monitoring` | Metrics, alerting | `roles/monitoring.viewer` | -## GenAI Toolbox (Google's MCP for Databases) +## GenAI Toolbox (Google's MCP for Databases) — opt-in Google's MCP Toolbox for Databases enables LLM agents to query Cloud SQL, AlloyDB, Spanner, BigQuery, and more safely. +> **Why this is not in `mcp.json`.** Toolbox needs `--config `, +> a path only you can supply. No Agent Plugins placeholder can express it, so +> shipping it in the portable manifest would mean a server that fails to start +> on every fresh install. Add it to your own client config once you have a +> `tools.yaml`. (A failing server is isolated by spec — it would not take the +> skills down — but a broken default is still a broken default.) + ```bash # Run the toolbox MCP server (requires a tools.yaml pointing at your database) # Tested 2026-07-23: this is the correct package. Full docs: https://mcp-toolbox.dev diff --git a/tests/skill-smoke-tests/test_skill_contract.py b/tests/skill-smoke-tests/test_skill_contract.py index def2dba..bf5e45d 100644 --- a/tests/skill-smoke-tests/test_skill_contract.py +++ b/tests/skill-smoke-tests/test_skill_contract.py @@ -133,6 +133,19 @@ def test_triggers_non_empty(skill_file: Path) -> None: assert triggers.strip(), f"{skill_file}: triggers must be non-empty" +@pytest.mark.parametrize("skill_file", skill_files(), ids=lambda p: p.parent.name) +def test_every_trigger_is_routable(skill_file: Path) -> None: + """A trigger absent from the description is unroutable — no client reads metadata.""" + fm = parse_frontmatter(skill_file) + description = fm["description"].lower() + triggers = [t.strip() for t in fm["metadata"][f"{NS}triggers"].split(",") if t.strip()] + unroutable = [t for t in triggers if t.lower() not in description] + assert not unroutable, ( + f"{skill_file}: trigger(s) {unroutable} appear only in metadata, so a " + f"conformant client routing on 'description' can never match them" + ) + + @pytest.mark.parametrize("skill_file", skill_files(), ids=lambda p: p.parent.name) def test_references_directory_exists(skill_file: Path) -> None: refs = skill_file.parent / "references"