test(agents): add SEP-2640 Skills conformance harness - #1655
test(agents): add SEP-2640 Skills conformance harness#1655groupthinking wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 PR Validation |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
There was a problem hiding this comment.
🟡 Changes recommended
A critical stale-approval flaw and multiple protocol-conformance gaps remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a fixture-only SEP-2640 MCP Skills conformance harness for future Agent Factory integration.
Changes:
- Models skill discovery, manifests, approvals, verification, and receipts.
- Adds tests covering protocol, integrity, and trust boundaries.
File summaries
| File | Description |
|---|---|
tests/unit/test_mcp_skills_conformance.py |
Exercises conformance and security behavior. |
src/youtube_extension/services/agents/mcp_skills_conformance.py |
Implements the in-memory conformance host. |
Review details
Suppressed comments (4)
src/youtube_extension/services/agents/mcp_skills_conformance.py:168
- Collision detection is confined to one server instance. The linked requirement also covers same-named skills from different MCP origins and local filesystem skills, but the two-origin test only compares identities and can never emit a collision receipt. Add a host-level registry/namespace fixture that combines origins and verifies no cross-origin shadowing or substitution.
for name, identities in names.items():
if len(identities) > 1:
for identity in identities:
src/youtube_extension/services/agents/mcp_skills_conformance.py:184
GetSkillResultalso requiresresultType: "complete", but this path ignores it, so malformedskills/getresponses pass conformance. Validate the result envelope before parsingskill.
entry = self._parse_entry(result.get("skill"))
src/youtube_extension/services/agents/mcp_skills_conformance.py:190
skills/getmust work for a URI absent from a partial listing and is also the specified refresh path after digest drift. Rejecting every unlisted or changed entry prevents both flows and leaves prior approval state untouched. Register an unlisted entry, and for an existing identity replace the snapshot while revoking approval when its resource fingerprint changes.
listed = self.entries.get(identity)
if listed is None or listed != entry:
raise SkillsConformanceError("skills/get disagrees with skills/list")
self._record("skills/get", "VERIFIED", entry.uri, "entry matches listing")
tests/unit/test_mcp_skills_conformance.py:79
- This test only proves that two tuple identities differ. Because the entries live in independent single-origin hosts, neither registry sees the same-name cross-origin collision and no collision receipt is emitted, leaving the linked requirement to surface and prevent substitution across servers or local skills unimplemented. Add a host-level multi-origin registry fixture and assert both entries remain explicitly addressable with a surfaced collision.
def test_compound_identity_keeps_same_uri_from_two_origins_distinct() -> None:
first = host().ingest_list({"skills": [raw_skill()]})[0]
second_host = SkillsConformanceHost("other-server", capabilities())
second = second_host.ingest_list({"skills": [raw_skill()]})[0]
assert first.identity != second.identity
- Files reviewed: 2/2 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| listed = self.entries.get(identity) | ||
| if listed is None or listed != entry: | ||
| raise SkillsConformanceError("skills/get disagrees with skills/list") |
| def directory_request(self, request_id: int, uri: str) -> dict[str, Any]: | ||
| if not self.directory_read_enabled: | ||
| raise SkillsConformanceError("directoryRead was not declared") | ||
| return { | ||
| "jsonrpc": "2.0", | ||
| "id": request_id, | ||
| "method": "resources/directory/read", | ||
| "params": {"uri": uri}, | ||
| } |
| raw_skills = result.get("skills") | ||
| if not isinstance(raw_skills, list): | ||
| raise SkillsConformanceError("skills/list result must contain skills") |
| for identity, old in self.entries.items(): | ||
| new = next_entries.get(identity) | ||
| if ( | ||
| identity in self.approvals | ||
| and (new is None or new.approval_fingerprint() != old.approval_fingerprint()) |
| if resource_uri == entry.uri: | ||
| parsed = _frontmatter(content) | ||
| if parsed != entry.frontmatter: | ||
| return self._record( | ||
| "resources/read", | ||
| "DENIED", | ||
| entry.uri, | ||
| "SKILL.md frontmatter differs from entry", | ||
| resource_uri, | ||
| resource.digest, | ||
| actual, | ||
| ) |
| if not isinstance(frontmatter.get("name"), str) or not isinstance( | ||
| frontmatter.get("description"), str | ||
| ): | ||
| raise SkillsConformanceError("frontmatter requires name and description") | ||
| if not _SKILL_NAME.fullmatch(frontmatter["name"]): | ||
| raise SkillsConformanceError("frontmatter name violates Agent Skills rules") |
| try: | ||
| block = text.split("---\n", 2)[1] | ||
| parsed = yaml.safe_load(block) | ||
| except (yaml.YAMLError, IndexError) as exc: | ||
| raise SkillsConformanceError("SKILL.md frontmatter is malformed") from exc |
groupthinking
added
high-priority
|
Please add a See governance: #898 |
September 10 stable-spec compatibility repairThe stable Skills specification changed after this PR was opened: ext-skills commit Normative fields now enforced on this branch:
Commits This remains fixture-only. Cache metadata is treated as a freshness/scope hint, never as integrity or approval evidence. Current-head security, CodeQL, dependency, and secret gates passed; CI and coverage are still running at the time of this receipt. No merge, runtime activation, remote skill load, or deployment was performed. |
Summary
Activates the pre-authorized TEST trigger in #1640 after SEP-2640 was accepted and official MCP Inspector support merged.
This is a fixture-only host conformance harness for Agent Factory. It does not enable remote skills in production or load external skill content.
What it tests
skills/list,skills/get,resources/read, and gatedresources/directory/readrequest shapes(host-assigned server identity, URI)skill://URI support as permitted by the extensionSources
Verification
Safety boundary
Draft only. No runtime registration, remote server connection, skill loading, tool execution, deployment, billing action, or production mutation is included. A later runtime adapter must preserve explicit approval and origin binding.
Advances #1640.