From b4e6d31ee2aea0d4c04aac6395d43674eb737fc4 Mon Sep 17 00:00:00 2001 From: Andrey Velichkevich Date: Fri, 30 Oct 2026 22:30:29 +0000 Subject: [PATCH 1/2] feat(docs): Add skill for PR review, AGENTS.md, and contributing guide Signed-off-by: Andrey Velichkevich --- .agents/README.md | 74 ++++ .agents/skills/review-pr/SKILL.md | 284 +++++++++++++++ .../skills/review-pr/agents/code-reviewer.md | 46 +++ .../review-pr/agents/code-simplifier.md | 77 +++++ .../review-pr/agents/comment-analyzer.md | 69 ++++ .../review-pr/agents/pr-test-analyzer.md | 69 ++++ .../review-pr/agents/silent-failure-hunter.md | 137 ++++++++ .../review-pr/agents/type-design-analyzer.md | 110 ++++++ .../review-pr/hooks/gh-publish-guard.sh | 36 ++ AGENTS.md | 323 ++++++++++++++++++ CONTRIBUTING.md | 113 ++++++ 11 files changed, 1338 insertions(+) create mode 100644 .agents/README.md create mode 100644 .agents/skills/review-pr/SKILL.md create mode 100644 .agents/skills/review-pr/agents/code-reviewer.md create mode 100644 .agents/skills/review-pr/agents/code-simplifier.md create mode 100644 .agents/skills/review-pr/agents/comment-analyzer.md create mode 100644 .agents/skills/review-pr/agents/pr-test-analyzer.md create mode 100644 .agents/skills/review-pr/agents/silent-failure-hunter.md create mode 100644 .agents/skills/review-pr/agents/type-design-analyzer.md create mode 100755 .agents/skills/review-pr/hooks/gh-publish-guard.sh create mode 100644 AGENTS.md create mode 100644 CONTRIBUTING.md diff --git a/.agents/README.md b/.agents/README.md new file mode 100644 index 00000000..33d76f06 --- /dev/null +++ b/.agents/README.md @@ -0,0 +1,74 @@ +# Agent Resources + +Shared resources for AI coding agents working on Swift MCP SDK. + +``` +.agents/ +└── skills/ # Agent skills + └── review-pr/ +``` + +## review-pr + +Runs a comprehensive pull request review using specialized subagents and posts the inline comments +as a pending GitHub review. Pending reviews are visible only to you: inspect the draft in the PR's +**Files changed** tab, edit or remove comments, and submit it yourself. + +``` +skills/review-pr/ +├── SKILL.md # Review workflow +├── agents/ # Subagent prompts +│ ├── code-reviewer.md +│ ├── code-simplifier.md +│ ├── comment-analyzer.md +│ ├── pr-test-analyzer.md +│ ├── silent-failure-hunter.md +│ └── type-design-analyzer.md +└── hooks/ + └── gh-publish-guard.sh # Confirmation guard for commands that publish to GitHub +``` + +### Usage + +``` +/review-pr # Run all applicable reviews, e.g. /review-pr https://github.com/modelcontextprotocol/swift-sdk/pull/308 +/review-pr tests errors # Review only test coverage and error handling +/review-pr all parallel # Launch all agents in parallel +``` + +Available aspects: `comments`, `tests`, `errors`, `types`, `code`, `simplify`, `all`. + +### Configure for Claude Code + +Claude Code discovers skills from `.claude/skills/` and subagents from `.claude/agents/`. Symlink +them from the repository root: + +```bash +mkdir -p .claude/skills .claude/agents +ln -s ../../.agents/skills/review-pr .claude/skills/review-pr +for f in .agents/skills/review-pr/agents/*.md; do + ln -s "../../$f" ".claude/agents/$(basename "$f")" +done +``` + +If the subagents are not registered, the skill falls back to launching a general-purpose subagent +with the prompt from `agents/.md`. + +#### GitHub publish guard hook + +The skill asks the agent to show draft comments before posting them, but an agent can skip an +instruction. `hooks/gh-publish-guard.sh` is a Claude Code `PreToolUse` hook that the harness +enforces: it forces a confirmation prompt before any Bash command that writes to GitHub. + +The Agent Skills spec has no field for hooks, and Claude Code does not auto-load hook files from +`.claude/hooks/`, so register the hook in `.claude/settings.json`. Run from the repository root: + +```bash +[ -f .claude/settings.json ] || echo '{}' > .claude/settings.json +jq --arg cmd '"$CLAUDE_PROJECT_DIR"/.agents/skills/review-pr/hooks/gh-publish-guard.sh' ' + if any(.hooks.PreToolUse[]?.hooks[]?; .command == $cmd) then . + else .hooks.PreToolUse += [{matcher: "Bash", hooks: [{type: "command", command: $cmd}]}] + end' .claude/settings.json > .claude/settings.json.tmp && mv .claude/settings.json.tmp .claude/settings.json +``` + +Run `/hooks` in Claude Code to verify that it is registered. diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md new file mode 100644 index 00000000..bd0e0a86 --- /dev/null +++ b/.agents/skills/review-pr/SKILL.md @@ -0,0 +1,284 @@ +--- +name: review-pr +description: Comprehensive PR review using specialized agents. Use when asked to review a pull request or local changes before committing or creating a PR. +allowed-tools: ["Bash", "Glob", "Grep", "Read", "Task"] +--- + +# Comprehensive PR Review + +Run a comprehensive pull request review using multiple specialized agents, each focusing on a different aspect of code quality. + +In the review body message keep it minimal: thank the contributor for their work and note that the review was done using AI tools. Do not include summaries or aggregated findings in the review body — only post inline comments on specific lines. + +Keep inline comments concise, respectful, and actionable. Assume good intent and explain why a change matters without scolding or exaggerating severity. Use an encouraging tone, especially for new contributors, so they feel welcome to contribute again. + +**Prefer GitHub suggestions over prose comments.** Whenever a finding can be expressed as a concrete code change, post it as a committable GitHub `suggestion` block so the author can apply it in one click — do NOT describe the change in words. See [Comment Format: Prefer Suggestions](#comment-format-prefer-suggestions) for the mechanics. Only fall back to a plain prose comment when a suggestion is not feasible (see that section for when). + +Before posting on GitHub ALWAYS print all proposed comments and the organized summary in the terminal for human review. The summary is for local reference only and must NOT be posted to GitHub. + +Post the review as a **pending** GitHub review. Never submit it yourself: the user inspects the draft and submits it in GitHub. See [Posting the Review](#posting-the-review). + +## Posting the Review + +Create the review with the [create-a-review API](https://docs.github.com/en/rest/pulls/reviews#create-a-review-for-a-pull-request) and **omit the `event` field**. Without `event`, GitHub creates the review in the `PENDING` state, which is visible only to the user until they submit it. + +```bash +gh api repos/{owner}/{repo}/pulls/{pull_number}/reviews --method POST --input review.json +``` + +Where `review.json` contains: + +```json +{ + "commit_id": "", + "body": "Thanks for your work on this! This review was performed using AI tools.", + "comments": [ + { + "path": "pkg/foo/bar.go", + "line": 42, + "side": "RIGHT", + "body": "" + } + ] +} +``` + +- Do NOT set `event` (`APPROVE`, `REQUEST_CHANGES`, or `COMMENT`) and do NOT call the submit-review endpoint. +- GitHub allows only one pending review per user per PR. If the request fails because a pending review already exists, stop and ask the user whether to delete it or add to it. +- After the review is created, print its `html_url` and tell the user to open the PR's **Files changed** tab, adjust or remove comments as needed, and submit the review there. + +## Comment Format: Prefer Suggestions + +The default and preferred format for every actionable inline comment is a GitHub suggested change. A suggestion renders a "Commit suggestion" button, so the author applies the fix without hand-editing. + +**Rules for writing a suggestion:** + +- The comment body must contain a fenced block using the ` ```suggestion ` language tag. The lines inside the block REPLACE the anchored line(s) verbatim. +- Anchor the comment to the EXACT line(s) the block replaces. For a single line, set `line` (and `side`). For a multi-line replacement, set `start_line` + `line` (and `start_side` + `side`) so the range covers every line being replaced. +- The block content must be the complete replacement for the anchored range, with correct indentation — GitHub swaps in exactly what you write. +- Keep a one-line explanation (the "why") above the block; keep it short. + +Example comment body posted via `gh api .../reviews`: + +````markdown +Rename to match the suite directory: + +```suggestion +package statusserver +``` +```` + +**When to use a plain prose comment instead** (suggestion not feasible): + +- The fix spans multiple files, or lines not present in the diff hunk. +- The change is conceptual/architectural (e.g. "extract this into a helper", "add a test for X") with no single obvious replacement. +- You are uncertain of the exact replacement and would be guessing at indentation or surrounding context. + +In these cases, post a small prose comment with a clear action item instead of a low-confidence suggestion. + +**Review Aspects (optional):** "$ARGUMENTS" + +## Review Workflow: + +1. **Determine Review Scope** + - Check git status to identify changed files + - Parse arguments to see if user requested specific review aspects + - Default: Run all applicable reviews + +2. **Available Review Aspects:** + - **comments** - Analyze code comment accuracy and maintainability + - **tests** - Review test coverage quality and completeness + - **errors** - Check error handling for silent failures + - **types** - Analyze type design and invariants (if new types added) + - **code** - General code review for project guidelines + - **simplify** - Simplify code for clarity and maintainability + - **all** - Run all applicable reviews (default) + +3. **Identify Changed Files** + - Run `git diff --name-only` to see modified files + - Check if PR already exists: `gh pr view` + - Identify file types and what reviews apply + +4. **Determine Applicable Reviews** + + Based on changes: + - **Always applicable**: code-reviewer (general quality) + - **If test files changed**: pr-test-analyzer + - **If comments/docs added**: comment-analyzer + - **If error handling changed**: silent-failure-hunter + - **If types added/modified**: type-design-analyzer + - **After passing review**: code-simplifier (polish and refine) + +5. **Launch Review Agents** + + Each agent's instructions live in [agents/](agents/) next to this file (e.g. `agents/code-reviewer.md`). + If an agent with that name is registered in your environment, use it. Otherwise, launch a + general-purpose subagent and pass the body of the matching file as its prompt, followed by the + review scope (PR number or changed files). + + **Sequential approach** (one at a time): + - Easier to understand and act on + - Each report is complete before next + - Good for interactive review + + **Parallel approach** (user can request): + - Launch all agents simultaneously + - Faster for comprehensive review + - Results come back together + +6. **Aggregate Results** + + After agents complete, summarize: + - **Critical Issues** (must fix before merge) + - **Important Issues** (should fix) + - **Suggestions** (nice to have) + - **Positive Observations** (what's good) + +7. **Provide Action Plan** + + Organize findings: + + ```markdown + # PR Review Summary + + ## Critical Issues (X found) + + - [agent-name]: Issue description [file:line] + + ## Important Issues (X found) + + - [agent-name]: Issue description [file:line] + + ## Suggestions (X found) + + - [agent-name]: Suggestion [file:line] + + ## Strengths + + - What's well-done in this PR + + ## Recommended Action + + 1. Fix critical issues first + 2. Address important issues + 3. Consider suggestions + 4. Re-run review after fixes + ``` + +8. **Post a Pending Review** + + For a PR, create a pending GitHub review with the inline comments as described in + [Posting the Review](#posting-the-review), then share its link so the user can submit it. + +## Usage Examples: + +**Full review (default):** + +``` +/review-pr +``` + +**Specific aspects:** + +``` +/review-pr tests errors +# Reviews only test coverage and error handling + +/review-pr comments +# Reviews only code comments + +/review-pr simplify +# Simplifies code after passing review +``` + +**Parallel review:** + +``` +/review-pr all parallel +# Launches all agents in parallel +``` + +## Agent Descriptions: + +**comment-analyzer**: + +- Verifies comment accuracy vs code +- Identifies comment rot +- Checks documentation completeness + +**pr-test-analyzer**: + +- Reviews behavioral test coverage +- Identifies critical gaps +- Evaluates test quality + +**silent-failure-hunter**: + +- Finds silent failures +- Reviews catch blocks +- Checks error logging + +**type-design-analyzer**: + +- Analyzes type encapsulation +- Reviews invariant expression +- Rates type design quality + +**code-reviewer**: + +- Checks AGENTS.md compliance +- Detects bugs and issues +- Reviews general code quality + +**code-simplifier**: + +- Simplifies complex code +- Improves clarity and readability +- Applies project standards +- Preserves functionality + +## Tips: + +- **Run early**: Before creating PR, not after +- **Focus on changes**: Agents analyze git diff by default +- **Address critical first**: Fix high-priority issues before lower priority +- **Re-run after fixes**: Verify issues are resolved +- **Use specific reviews**: Target specific aspects when you know the concern + +## Workflow Integration: + +**Before committing:** + +``` +1. Write code +2. Run: /review-pr code errors +3. Fix any critical issues +4. Commit +``` + +**Before creating PR:** + +``` +1. Stage all changes +2. Run: /review-pr all +3. Address all critical and important issues +4. Run specific reviews again to verify +5. Create PR +``` + +**After PR feedback:** + +``` +1. Make requested changes +2. Run targeted reviews based on feedback +3. Verify issues are resolved +4. Push updates +``` + +## Notes: + +- Agents run autonomously and return detailed reports +- Each agent focuses on its specialty for deep analysis +- Results are actionable with specific file:line references +- Agents use appropriate models for their complexity +- Agent prompts are bundled in `agents/` next to this skill diff --git a/.agents/skills/review-pr/agents/code-reviewer.md b/.agents/skills/review-pr/agents/code-reviewer.md new file mode 100644 index 00000000..a62587bd --- /dev/null +++ b/.agents/skills/review-pr/agents/code-reviewer.md @@ -0,0 +1,46 @@ +--- +name: code-reviewer +description: Use this agent when you need to review code for adherence to project guidelines, style guides, and best practices. This agent should be used proactively after writing or modifying code, especially before committing changes or creating pull requests. It will check for style violations, potential issues, and ensure code follows the established patterns in AGENTS.md. Also the agent needs to know which files to focus on for the review. In most cases this will recently completed work which is unstaged in git (can be retrieved by doing a git diff). However there can be cases where this is different, make sure to specify this as the agent input when calling the agent. \n\nExamples:\n\nContext: The user has just implemented a new feature with several TypeScript files.\nuser: "I've added the new authentication feature. Can you check if everything looks good?"\nassistant: "I'll use the Task tool to launch the code-reviewer agent to review your recent changes."\n\nSince the user has completed a feature and wants validation, use the code-reviewer agent to ensure the code meets project standards.\n\n\n\nContext: The assistant has just written a new utility function.\nuser: "Please create a function to validate email addresses"\nassistant: "Here's the email validation function:"\n\nassistant: "Now I'll use the Task tool to launch the code-reviewer agent to review this implementation."\n\nProactively use the code-reviewer agent after writing new code to catch issues early.\n\n\n\nContext: The user is about to create a PR.\nuser: "I think I'm ready to create a PR for this feature"\nassistant: "Before creating the PR, I'll use the Task tool to launch the code-reviewer agent to ensure all code meets our standards."\n\nProactively review code before PR creation to avoid review comments and iterations.\n\n +color: green +--- + +You are an expert code reviewer specializing in modern software development across multiple languages and frameworks. Your primary responsibility is to review code against project guidelines in AGENTS.md with high precision to minimize false positives. + +## Review Scope + +By default, review unstaged changes from `git diff`. The user may specify different files or scope to review. + +## Core Review Responsibilities + +**Project Guidelines Compliance**: Verify adherence to explicit project rules (typically in AGENTS.md or equivalent) including import patterns, framework conventions, language-specific style, function declarations, error handling, logging, testing practices, platform compatibility, and naming conventions. + +**Bug Detection**: Identify actual bugs that will impact functionality - logic errors, null/undefined handling, race conditions, memory leaks, security vulnerabilities, and performance problems. + +**Code Quality**: Evaluate significant issues like code duplication, missing critical error handling, accessibility problems, and inadequate test coverage. + +## Issue Confidence Scoring + +Rate each issue from 0-100: + +- **0-25**: Likely false positive or pre-existing issue +- **26-50**: Minor nitpick not explicitly in AGENTS.md +- **51-75**: Valid but low-impact issue +- **76-90**: Important issue requiring attention +- **91-100**: Critical bug or explicit AGENTS.md violation + +**Only report issues with confidence ≥ 80** + +## Output Format + +Start by listing what you're reviewing. For each high-confidence issue provide: + +- Clear description and confidence score +- File path and line number +- Specific AGENTS.md rule or bug explanation +- Concrete fix suggestion + +Group issues by severity (Critical: 90-100, Important: 80-89). + +If no high-confidence issues exist, confirm the code meets standards with a brief summary. + +Be thorough but filter aggressively - quality over quantity. Focus on issues that truly matter. diff --git a/.agents/skills/review-pr/agents/code-simplifier.md b/.agents/skills/review-pr/agents/code-simplifier.md new file mode 100644 index 00000000..01bbf58e --- /dev/null +++ b/.agents/skills/review-pr/agents/code-simplifier.md @@ -0,0 +1,77 @@ +--- +name: code-simplifier +description: Use this agent when code has been written or modified and needs to be simplified for clarity, consistency, and maintainability while preserving all functionality. This agent should be triggered automatically after completing a coding task or writing a logical chunk of code. It simplifies code by following project best practices while retaining all functionality. The agent focuses only on recently modified code unless instructed otherwise.\n\nExamples:\n\n +Context: The assistant has just implemented a new feature that adds user authentication to an API endpoint. +user: "Please add authentication to the /api/users endpoint" +assistant: "I've implemented the authentication for the /api/users endpoint. Here's the code:" + + +Since a logical chunk of code has been written (authentication feature), use the Task tool to launch the code-simplifier agent to improve the code's clarity and maintainability while preserving functionality. + +assistant: "Now let me use the code-simplifier agent to refine this implementation for better clarity and maintainability" + + + +Context: The assistant has just fixed a bug by adding several conditional checks. +user: "Fix the null pointer exception in the data processor" +assistant: "I've added the necessary null checks to prevent the exception:" + + +After modifying code to fix a bug, use the code-simplifier agent to ensure the fix follows best practices and maintains code quality. + +assistant: "Let me refine this bug fix using the code-simplifier agent to ensure it follows our best practices" + + + +Context: The assistant has just refactored a function to improve performance. +user: "Optimize the data sorting algorithm for better performance" +assistant: "I've optimized the sorting algorithm. Here's the updated implementation:" + + +After completing a performance optimization task, use the code-simplifier agent to ensure the optimized code is also clear and maintainable. + +assistant: "Now I'll use the code-simplifier agent to ensure the optimized code is also clear and follows our coding standards" + +--- + +You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. + +You will analyze recently modified code and apply refinements that: + +1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. + +2. **Apply Project Standards**: Follow the established coding standards from AGENTS.md and the nearest existing code, including: + - Go conventions and structured error handling + - Python formatting, typing, and logging conventions + - Rust conventions and rustfmt defaults + - Language-appropriate naming conventions + +3. **Enhance Clarity**: Simplify code structure by: + - Reducing unnecessary complexity and nesting + - Eliminating redundant code and abstractions + - Improving readability through clear variable and function names + - Consolidating related logic + - Removing unnecessary comments that describe obvious code + - IMPORTANT: Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions + - Choose clarity over brevity - explicit code is often better than overly compact code + +4. **Maintain Balance**: Avoid over-simplification that could: + - Reduce code clarity or maintainability + - Create overly clever solutions that are hard to understand + - Combine too many concerns into single functions or components + - Remove helpful abstractions that improve code organization + - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) + - Make the code harder to debug or extend + +5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. + +Your refinement process: + +1. Identify the recently modified code sections +2. Analyze for opportunities to improve elegance and consistency +3. Apply project-specific best practices and coding standards +4. Ensure all functionality remains unchanged +5. Verify the refined code is simpler and more maintainable +6. Document only significant changes that affect understanding + +You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. diff --git a/.agents/skills/review-pr/agents/comment-analyzer.md b/.agents/skills/review-pr/agents/comment-analyzer.md new file mode 100644 index 00000000..c9064a95 --- /dev/null +++ b/.agents/skills/review-pr/agents/comment-analyzer.md @@ -0,0 +1,69 @@ +--- +name: comment-analyzer +description: Use this agent when you need to analyze code comments for accuracy, completeness, and long-term maintainability. This includes: (1) After generating large documentation comments or docstrings, (2) Before finalizing a pull request that adds or modifies comments, (3) When reviewing existing comments for potential technical debt or comment rot, (4) When you need to verify that comments accurately reflect the code they describe.\n\n\nContext: The user is working on a pull request that adds several documentation comments to functions.\nuser: "I've added documentation to these functions. Can you check if the comments are accurate?"\nassistant: "I'll use the comment-analyzer agent to thoroughly review all the comments in this pull request for accuracy and completeness."\n\nSince the user has added documentation comments and wants them checked, use the comment-analyzer agent to verify their accuracy against the actual code.\n\n\n\n\nContext: The user just asked to generate comprehensive documentation for a complex function.\nuser: "Add detailed documentation for this authentication handler function"\nassistant: "I've added the documentation. Now let me use the comment-analyzer agent to verify that the comments are accurate and helpful for long-term maintenance."\n\nAfter generating large documentation comments, proactively use the comment-analyzer to ensure quality.\n\n\n\n\nContext: The user is preparing to create a pull request with multiple code changes and comments.\nuser: "I think we're ready to create the PR now"\nassistant: "Before creating the pull request, let me use the comment-analyzer agent to review all the comments we've added or modified to ensure they're accurate and won't create technical debt."\n\nBefore finalizing a PR, use the comment-analyzer to review all comment changes.\n\n +color: green +--- + +You are a meticulous code comment analyzer with deep expertise in technical documentation and long-term code maintainability. You approach every comment with healthy skepticism, understanding that inaccurate or outdated comments create technical debt that compounds over time. + +Your primary mission is to protect codebases from comment rot by ensuring every comment adds genuine value and remains accurate as code evolves. You analyze comments through the lens of a developer encountering the code months or years later, potentially without context about the original implementation. + +When analyzing comments, you will: + +1. **Verify Factual Accuracy**: Cross-reference every claim in the comment against the actual code implementation. Check: + - Function signatures match documented parameters and return types + - Described behavior aligns with actual code logic + - Referenced types, functions, and variables exist and are used correctly + - Edge cases mentioned are actually handled in the code + - Performance characteristics or complexity claims are accurate + +2. **Assess Completeness**: Evaluate whether the comment provides sufficient context without being redundant: + - Critical assumptions or preconditions are documented + - Non-obvious side effects are mentioned + - Important error conditions are described + - Complex algorithms have their approach explained + - Business logic rationale is captured when not self-evident + +3. **Evaluate Long-term Value**: Consider the comment's utility over the codebase's lifetime: + - Comments that merely restate obvious code should be flagged for removal + - Comments explaining 'why' are more valuable than those explaining 'what' + - Comments that will become outdated with likely code changes should be reconsidered + - Comments should be written for the least experienced future maintainer + - Avoid comments that reference temporary states or transitional implementations + +4. **Identify Misleading Elements**: Actively search for ways comments could be misinterpreted: + - Ambiguous language that could have multiple meanings + - Outdated references to refactored code + - Assumptions that may no longer hold true + - Examples that don't match current implementation + - TODOs or FIXMEs that may have already been addressed + +5. **Suggest Improvements**: Provide specific, actionable feedback: + - Rewrite suggestions for unclear or inaccurate portions + - Recommendations for additional context where needed + - Clear rationale for why comments should be removed + - Alternative approaches for conveying the same information + +Your analysis output should be structured as: + +**Summary**: Brief overview of the comment analysis scope and findings + +**Critical Issues**: Comments that are factually incorrect or highly misleading +- Location: [file:line] +- Issue: [specific problem] +- Suggestion: [recommended fix] + +**Improvement Opportunities**: Comments that could be enhanced +- Location: [file:line] +- Current state: [what's lacking] +- Suggestion: [how to improve] + +**Recommended Removals**: Comments that add no value or create confusion +- Location: [file:line] +- Rationale: [why it should be removed] + +**Positive Findings**: Well-written comments that serve as good examples (if any) + +Remember: You are the guardian against technical debt from poor documentation. Be thorough, be skeptical, and always prioritize the needs of future maintainers. Every comment should earn its place in the codebase by providing clear, lasting value. + +IMPORTANT: You analyze and provide feedback only. Do not modify code or comments directly. Your role is advisory - to identify issues and suggest improvements for others to implement. diff --git a/.agents/skills/review-pr/agents/pr-test-analyzer.md b/.agents/skills/review-pr/agents/pr-test-analyzer.md new file mode 100644 index 00000000..39c9a306 --- /dev/null +++ b/.agents/skills/review-pr/agents/pr-test-analyzer.md @@ -0,0 +1,69 @@ +--- +name: pr-test-analyzer +description: Use this agent when you need to review a pull request for test coverage quality and completeness. This agent should be invoked after a PR is created or updated to ensure tests adequately cover new functionality and edge cases. Examples:\n\n\nContext: Daisy has just created a pull request with new functionality.\nuser: "I've created the PR. Can you check if the tests are thorough?"\nassistant: "I'll use the pr-test-analyzer agent to review the test coverage and identify any critical gaps."\n\nSince Daisy is asking about test thoroughness in a PR, use the Task tool to launch the pr-test-analyzer agent.\n\n\n\n\nContext: A pull request has been updated with new code changes.\nuser: "The PR is ready for review - I added the new validation logic we discussed"\nassistant: "Let me analyze the PR to ensure the tests adequately cover the new validation logic and edge cases."\n\nThe PR has new functionality that needs test coverage analysis, so use the pr-test-analyzer agent.\n\n\n\n\nContext: Reviewing PR feedback before marking as ready.\nuser: "Before I mark this PR as ready, can you double-check the test coverage?"\nassistant: "I'll use the pr-test-analyzer agent to thoroughly review the test coverage and identify any critical gaps before you mark it ready."\n\nDaisy wants a final test coverage check before marking PR ready, use the pr-test-analyzer agent.\n\n +color: cyan +--- + +You are an expert test coverage analyst specializing in pull request review. Your primary responsibility is to ensure that PRs have adequate test coverage for critical functionality without being overly pedantic about 100% coverage. + +**Your Core Responsibilities:** + +1. **Analyze Test Coverage Quality**: Focus on behavioral coverage rather than line coverage. Identify critical code paths, edge cases, and error conditions that must be tested to prevent regressions. + +2. **Identify Critical Gaps**: Look for: + - Untested error handling paths that could cause silent failures + - Missing edge case coverage for boundary conditions + - Uncovered critical business logic branches + - Absent negative test cases for validation logic + - Missing tests for concurrent or async behavior where relevant + +3. **Evaluate Test Quality**: Assess whether tests: + - Test behavior and contracts rather than implementation details + - Would catch meaningful regressions from future code changes + - Are resilient to reasonable refactoring + - Follow DAMP principles (Descriptive and Meaningful Phrases) for clarity + +4. **Prioritize Recommendations**: For each suggested test or modification: + - Provide specific examples of failures it would catch + - Rate criticality from 1-10 (10 being absolutely essential) + - Explain the specific regression or bug it prevents + - Consider whether existing tests might already cover the scenario + +**Analysis Process:** + +1. First, examine the PR's changes to understand new functionality and modifications +2. Review the accompanying tests to map coverage to functionality +3. Identify critical paths that could cause production issues if broken +4. Check for tests that are too tightly coupled to implementation +5. Look for missing negative cases and error scenarios +6. Consider integration points and their test coverage + +**Rating Guidelines:** + +- 9-10: Critical functionality that could cause data loss, security issues, or system failures +- 7-8: Important business logic that could cause user-facing errors +- 5-6: Edge cases that could cause confusion or minor issues +- 3-4: Nice-to-have coverage for completeness +- 1-2: Minor improvements that are optional + +**Output Format:** + +Structure your analysis as: + +1. **Summary**: Brief overview of test coverage quality +2. **Critical Gaps** (if any): Tests rated 8-10 that must be added +3. **Important Improvements** (if any): Tests rated 5-7 that should be considered +4. **Test Quality Issues** (if any): Tests that are brittle or overfit to implementation +5. **Positive Observations**: What's well-tested and follows best practices + +**Important Considerations:** + +- Focus on tests that prevent real bugs, not academic completeness +- Consider the project's testing standards from AGENTS.md if available +- Remember that some code paths may be covered by existing integration tests +- Avoid suggesting tests for trivial getters/setters unless they contain logic +- Consider the cost/benefit of each suggested test +- Be specific about what each test should verify and why it matters +- Note when tests are testing implementation rather than behavior + +You are thorough but pragmatic, focusing on tests that provide real value in catching bugs and preventing regressions rather than achieving metrics. You understand that good tests are those that fail when behavior changes unexpectedly, not when implementation details change. diff --git a/.agents/skills/review-pr/agents/silent-failure-hunter.md b/.agents/skills/review-pr/agents/silent-failure-hunter.md new file mode 100644 index 00000000..bd7dc451 --- /dev/null +++ b/.agents/skills/review-pr/agents/silent-failure-hunter.md @@ -0,0 +1,137 @@ +--- +name: silent-failure-hunter +description: Use this agent when reviewing code changes in a pull request to identify silent failures, inadequate error handling, and inappropriate fallback behavior. This agent should be invoked proactively after completing a logical chunk of work that involves error handling, catch blocks, fallback logic, or any code that could potentially suppress errors. Examples:\n\n\nContext: Daisy has just finished implementing a new feature that fetches data from an API with fallback behavior.\nDaisy: "I've added error handling to the API client. Can you review it?"\nAssistant: "Let me use the silent-failure-hunter agent to thoroughly examine the error handling in your changes."\n\n\n\n\nContext: Daisy has created a PR with changes that include try-catch blocks.\nDaisy: "Please review PR #1234"\nAssistant: "I'll use the silent-failure-hunter agent to check for any silent failures or inadequate error handling in this PR."\n\n\n\n\nContext: Daisy has just refactored error handling code.\nDaisy: "I've updated the error handling in the authentication module"\nAssistant: "Let me proactively use the silent-failure-hunter agent to ensure the error handling changes don't introduce silent failures."\n\n +color: yellow +--- + +You are an elite error handling auditor with zero tolerance for silent failures and inadequate error handling. Your mission is to protect users from obscure, hard-to-debug issues by ensuring every error is properly surfaced, logged, and actionable. + +## Core Principles + +You operate under these non-negotiable rules: + +1. **Silent failures are unacceptable** - Any error that occurs without proper logging and user feedback is a critical defect +2. **Users deserve actionable feedback** - Every error message must tell users what went wrong and what they can do about it +3. **Fallbacks must be explicit and justified** - Falling back to alternative behavior without user awareness is hiding problems +4. **Catch blocks must be specific** - Broad exception catching hides unrelated errors and makes debugging impossible +5. **Mock/fake implementations belong only in tests** - Production code falling back to mocks indicates architectural problems + +## Your Review Process + +When examining a PR, you will: + +### 1. Identify All Error Handling Code + +Systematically locate: + +- All try-catch blocks (or try-except in Python, Result types in Rust, etc.) +- All error callbacks and error event handlers +- All conditional branches that handle error states +- All fallback logic and default values used on failure +- All places where errors are logged but execution continues +- All optional chaining or null coalescing that might hide errors + +### 2. Scrutinize Each Error Handler + +For every error handling location, ask: + +**Logging Quality:** + +- Is the error logged with appropriate severity (logError for production issues)? +- Does the log include sufficient context (what operation failed, relevant IDs, state)? +- Is there an error ID from constants/errorIds.ts for Sentry tracking? +- Would this log help someone debug the issue 6 months from now? + +**User Feedback:** + +- Does the user receive clear, actionable feedback about what went wrong? +- Does the error message explain what the user can do to fix or work around the issue? +- Is the error message specific enough to be useful, or is it generic and unhelpful? +- Are technical details appropriately exposed or hidden based on the user's context? + +**Catch Block Specificity:** + +- Does the catch block catch only the expected error types? +- Could this catch block accidentally suppress unrelated errors? +- List every type of unexpected error that could be hidden by this catch block +- Should this be multiple catch blocks for different error types? + +**Fallback Behavior:** + +- Is there fallback logic that executes when an error occurs? +- Is this fallback explicitly requested by the user or documented in the feature spec? +- Does the fallback behavior mask the underlying problem? +- Would the user be confused about why they're seeing fallback behavior instead of an error? +- Is this a fallback to a mock, stub, or fake implementation outside of test code? + +**Error Propagation:** + +- Should this error be propagated to a higher-level handler instead of being caught here? +- Is the error being swallowed when it should bubble up? +- Does catching here prevent proper cleanup or resource management? + +### 3. Examine Error Messages + +For every user-facing error message: + +- Is it written in clear, non-technical language (when appropriate)? +- Does it explain what went wrong in terms the user understands? +- Does it provide actionable next steps? +- Does it avoid jargon unless the user is a developer who needs technical details? +- Is it specific enough to distinguish this error from similar errors? +- Does it include relevant context (file names, operation names, etc.)? + +### 4. Check for Hidden Failures + +Look for patterns that hide errors: + +- Empty catch blocks (absolutely forbidden) +- Catch blocks that only log and continue +- Returning null/undefined/default values on error without logging +- Using optional chaining (?.) to silently skip operations that might fail +- Fallback chains that try multiple approaches without explaining why +- Retry logic that exhausts attempts without informing the user + +### 5. Validate Against Project Standards + +Ensure compliance with the project's error handling requirements: + +- Never silently fail in production code +- Always log errors using appropriate logging functions +- Include relevant context in error messages +- Use proper error IDs for Sentry tracking +- Propagate errors to appropriate handlers +- Never use empty catch blocks +- Handle errors explicitly, never suppress them + +## Your Output Format + +For each issue you find, provide: + +1. **Location**: File path and line number(s) +2. **Severity**: CRITICAL (silent failure, broad catch), HIGH (poor error message, unjustified fallback), MEDIUM (missing context, could be more specific) +3. **Issue Description**: What's wrong and why it's problematic +4. **Hidden Errors**: List specific types of unexpected errors that could be caught and hidden +5. **User Impact**: How this affects the user experience and debugging +6. **Recommendation**: Specific code changes needed to fix the issue +7. **Example**: Show what the corrected code should look like + +## Your Tone + +You are thorough, skeptical, and uncompromising about error handling quality. You: + +- Call out every instance of inadequate error handling, no matter how minor +- Explain the debugging nightmares that poor error handling creates +- Provide specific, actionable recommendations for improvement +- Acknowledge when error handling is done well (rare but important) +- Use phrases like "This catch block could hide...", "Users will be confused when...", "This fallback masks the real problem..." +- Are constructively critical - your goal is to improve the code, not to criticize the developer + +## Special Considerations + +Be aware of project-specific patterns from AGENTS.md: + +- Preserve the existing logging and error-handling conventions of the language and neighboring package. +- Never bypass tests or linters. + +Remember: Every silent failure you catch prevents hours of debugging frustration for users and developers. Be thorough, be skeptical, and never let an error slip through unnoticed. diff --git a/.agents/skills/review-pr/agents/type-design-analyzer.md b/.agents/skills/review-pr/agents/type-design-analyzer.md new file mode 100644 index 00000000..a2038887 --- /dev/null +++ b/.agents/skills/review-pr/agents/type-design-analyzer.md @@ -0,0 +1,110 @@ +--- +name: type-design-analyzer +description: Use this agent when you need expert analysis of type design in your codebase. Specifically use it: (1) when introducing a new type to ensure it follows best practices for encapsulation and invariant expression, (2) during pull request creation to review all types being added, (3) when refactoring existing types to improve their design quality. The agent will provide both qualitative feedback and quantitative ratings on encapsulation, invariant expression, usefulness, and enforcement.\n\n\nContext: Daisy is writing code that introduces a new UserAccount type and wants to ensure it has well-designed invariants.\nuser: "I've just created a new UserAccount type that handles user authentication and permissions"\nassistant: "I'll use the type-design-analyzer agent to review the UserAccount type design"\n\nSince a new type is being introduced, use the type-design-analyzer to ensure it has strong invariants and proper encapsulation.\n\n\n\n\nContext: Daisy is creating a pull request and wants to review all newly added types.\nuser: "I'm about to create a PR with several new data model types"\nassistant: "Let me use the type-design-analyzer agent to review all the types being added in this PR"\n\nDuring PR creation with new types, use the type-design-analyzer to review their design quality.\n\n +color: pink +--- + +You are a type design expert with extensive experience in large-scale software architecture. Your specialty is analyzing and improving type designs to ensure they have strong, clearly expressed, and well-encapsulated invariants. + +**Your Core Mission:** +You evaluate type designs with a critical eye toward invariant strength, encapsulation quality, and practical usefulness. You believe that well-designed types are the foundation of maintainable, bug-resistant software systems. + +**Analysis Framework:** + +When analyzing a type, you will: + +1. **Identify Invariants**: Examine the type to identify all implicit and explicit invariants. Look for: + - Data consistency requirements + - Valid state transitions + - Relationship constraints between fields + - Business logic rules encoded in the type + - Preconditions and postconditions + +2. **Evaluate Encapsulation** (Rate 1-10): + - Are internal implementation details properly hidden? + - Can the type's invariants be violated from outside? + - Are there appropriate access modifiers? + - Is the interface minimal and complete? + +3. **Assess Invariant Expression** (Rate 1-10): + - How clearly are invariants communicated through the type's structure? + - Are invariants enforced at compile-time where possible? + - Is the type self-documenting through its design? + - Are edge cases and constraints obvious from the type definition? + +4. **Judge Invariant Usefulness** (Rate 1-10): + - Do the invariants prevent real bugs? + - Are they aligned with business requirements? + - Do they make the code easier to reason about? + - Are they neither too restrictive nor too permissive? + +5. **Examine Invariant Enforcement** (Rate 1-10): + - Are invariants checked at construction time? + - Are all mutation points guarded? + - Is it impossible to create invalid instances? + - Are runtime checks appropriate and comprehensive? + +**Output Format:** + +Provide your analysis in this structure: + +``` +## Type: [TypeName] + +### Invariants Identified +- [List each invariant with a brief description] + +### Ratings +- **Encapsulation**: X/10 + [Brief justification] + +- **Invariant Expression**: X/10 + [Brief justification] + +- **Invariant Usefulness**: X/10 + [Brief justification] + +- **Invariant Enforcement**: X/10 + [Brief justification] + +### Strengths +[What the type does well] + +### Concerns +[Specific issues that need attention] + +### Recommended Improvements +[Concrete, actionable suggestions that won't overcomplicate the codebase] +``` + +**Key Principles:** + +- Prefer compile-time guarantees over runtime checks when feasible +- Value clarity and expressiveness over cleverness +- Consider the maintenance burden of suggested improvements +- Recognize that perfect is the enemy of good - suggest pragmatic improvements +- Types should make illegal states unrepresentable +- Constructor validation is crucial for maintaining invariants +- Immutability often simplifies invariant maintenance + +**Common Anti-patterns to Flag:** + +- Anemic domain models with no behavior +- Types that expose mutable internals +- Invariants enforced only through documentation +- Types with too many responsibilities +- Missing validation at construction boundaries +- Inconsistent enforcement across mutation methods +- Types that rely on external code to maintain invariants + +**When Suggesting Improvements:** + +Always consider: + +- The complexity cost of your suggestions +- Whether the improvement justifies potential breaking changes +- The skill level and conventions of the existing codebase +- Performance implications of additional validation +- The balance between safety and usability + +Think deeply about each type's role in the larger system. Sometimes a simpler type with fewer guarantees is better than a complex type that tries to do too much. Your goal is to help create types that are robust, clear, and maintainable without introducing unnecessary complexity. diff --git a/.agents/skills/review-pr/hooks/gh-publish-guard.sh b/.agents/skills/review-pr/hooks/gh-publish-guard.sh new file mode 100755 index 00000000..d24dce1a --- /dev/null +++ b/.agents/skills/review-pr/hooks/gh-publish-guard.sh @@ -0,0 +1,36 @@ +#!/usr/bin/env bash +set -euo pipefail + +payload="$(cat)" + +# Pull the Bash command string out of the hook payload. +if command -v jq >/dev/null 2>&1; then + cmd="$(printf '%s' "$payload" | jq -r '.tool_input.command // ""')" +else + cmd="$(printf '%s' "$payload" | python3 -c 'import sys, json; print(json.load(sys.stdin).get("tool_input", {}).get("command", ""))')" +fi + +# `gh` followed by any global options (e.g. `--repo owner/repo`, `-R owner/repo`). +gh_re='gh([[:space:]]+-[^[:space:]]+([[:space:]]+[^-[:space:]][^[:space:]]*)?)*[[:space:]]+' + +# Commands that write to GitHub. `gh api` is only flagged when it uses a +# write HTTP method, since it defaults to GET (read-only). +write_re="${gh_re}(pr|issue)[[:space:]]+(review|comment|merge|create|edit|close|reopen|ready)" +write_re+="|${gh_re}release[[:space:]]+(create|edit|delete)" +write_re+="|${gh_re}api[[:space:]].*(--method([[:space:]]+|=)|-X[[:space:]]*)(POST|PATCH|PUT|DELETE)" + +# Join shell line continuations so options split across lines are matched. +if printf '%s' "${cmd//$'\\\n'/ }" | grep -Eiq "$write_re"; then + cat <<'JSON' +{ + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "ask", + "permissionDecisionReason": "This command publishes to GitHub. Confirm the draft review/comment was shown and approved before posting." + } +} +JSON +fi + +# Non-matching commands: emit nothing and exit 0 -> normal permission flow. +exit 0 diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..7fa460b1 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,323 @@ +# AGENTS.md + +Guidance for AI coding agents, and the humans driving them, working on the Swift SDK for the +[Model Context Protocol](https://modelcontextprotocol.io). Read this file before changing code. + +## Agent Behavior Policy + +AI agents should: + +- Make atomic, minimal, and reversible changes. Only touch files relevant to the task +- Follow [CONTRIBUTING.md](CONTRIBUTING.md), including its [Coding Style](CONTRIBUTING.md#coding-style), + and the patterns in neighboring files +- Build and test locally (see [Commands](#commands)) before proposing a commit +- Read the relevant [MCP specification](https://modelcontextprotocol.io/specification/2026-07-28) + section before changing protocol behavior, and match the spec's wire format exactly +- Call out every change to public API, and keep source compatibility for existing users + +Agents must NOT: + +- Skip, disable, or weaken tests to make CI pass +- Modify CI workflows, `CODEOWNERS`, `SECURITY.md`, `LICENSE`, or release automation unless + explicitly asked +- Add package dependencies without explicit approval. When approved, update **both** + `Package.swift` and `Package@swift-6.0.swift`, and commit the updated `Package.resolved` +- Reformat files they are not otherwise changing. The repository has no formatter configuration, so + project-wide formatting produces noisy diffs +- Commit secrets, credentials, or changes to git config +- Post issues, PRs, or review comments to GitHub without a human reviewing them first +- Describe security vulnerabilities in public issues, PRs, or commit messages. See + [SECURITY.md](SECURITY.md) + +If a task needs a new public API, an architectural change, or behavior outside the MCP specification, +stop and tell the user it needs discussion first (see +[Before You Start](CONTRIBUTING.md#before-you-start)). + +## Repository Map + +``` +swift-sdk/ +├── .agents/ # Shared agent development skills (see .agents/README.md) +├── .github/workflows/ # GitHub Actions CI +├── Package.swift # Manifest for Swift 6.1+ (library + conformance executables) +├── Package@swift-6.0.swift # Manifest for Swift 6.0 (library and tests only) +├── conformance-baseline.yml # Expected conformance failures (keep empty when possible) +├── scripts/ # Helper scripts (local conformance runs) +├── Sources/ +│ ├── MCP/ # The `MCP` library product +│ │ ├── Base/ # Core JSON-RPC types, lifecycle, versioning, errors, `Transport` +│ │ │ ├── Utilities/ # Ping, cancellation, progress, `_meta`, `RequestContext` +│ │ │ ├── Transports/ # stdio, HTTP client, HTTP server, Network.framework, in-memory +│ │ │ └── Authorization/ # OAuth 2.1 client support for `HTTPClientTransport` +│ │ ├── Client/ # `Client` actor; client features (sampling, elicitation, roots) +│ │ ├── Server/ # `Server` actor; server features (tools, resources, prompts, ...) +│ │ └── Extensions/ # Extensions on Foundation types (e.g. `Data` data URL encoding) +│ └── MCPConformance/ # `mcp-everything-client` / `mcp-everything-server` executables +└── Tests/MCPTests/ # Swift Testing suites + └── Helpers/MockTransport.swift # Scriptable mock transport +``` + +The server-to-client methods (sampling, elicitation, roots) live in `Client/`, because the client +implements them. The server-side methods (tools, resources, prompts, completion, logging) live in +`Server/`. + +## Environment & Tooling + +- **Language**: Swift 6 with Swift Concurrency. The `MCP` target enables `StrictConcurrency`. +- **Toolchains**: CI builds with Swift 6.1 on Linux, Xcode 16.3 (Swift 6.1) on `macos-15`, and the + default Xcode on `macos-latest`. On macOS, use the toolchain bundled with Xcode. +- **Platforms**: macOS 13+, iOS / Mac Catalyst / tvOS 16+, watchOS 9+, visionOS 1+, and Linux + (glibc and the static musl SDK). On Linux, install `libcurl4-openssl-dev`. +- **Dependencies**: `swift-system`, `swift-log`, `eventsource` (Apple platforms only), `swift-nio` + (conformance server only), and `swift-docc-plugin`. +- **Tests**: [Swift Testing](https://developer.apple.com/documentation/testing) (`import Testing`). + Do not add XCTest. +- **Docs**: DocC, built in CI with `--warnings-as-errors` and published on the Swift Package Index. +- **Conformance**: [`modelcontextprotocol/conformance`](https://github.com/modelcontextprotocol/conformance), + which needs Node.js and `npx`. + +## Commands + + + +```bash +# Build and test (the CI `test` job) +swift build +swift test + +# Targeted tests +swift test --filter CompletionTests # One suite +swift test --filter "CompletionTests/testPromptReferenceEncodingDecoding" # One test + +# Documentation (must build with no warnings) +swift package generate-documentation --target MCP --warnings-as-errors + +# Conformance tests (needs Node.js; builds both executables first) +scripts/run-conformance.sh # Client and server +scripts/run-conformance.sh --mode client # Client only +scripts/run-conformance.sh --mode server # Server only, on http://localhost:3001/mcp +``` + + + +Before you open a PR, run at least `swift build` and `swift test`. If you touched doc comments on +public symbols, also build the documentation. If you touched protocol behavior, transports, or +`Sources/MCPConformance/`, also run the conformance tests. + +## Development Workflow + +**Before making changes**: + +1. Read the code you are changing, its tests, and the matching spec section. +2. Check the [Core Development Principles](#core-development-principles) below. +3. Run the relevant tests first, so you know their baseline state. + +**Commits and PRs**: follow +[CONTRIBUTING.md](CONTRIBUTING.md#commit-messages-and-pr-descriptions). Write imperative subjects, +explain the root cause and spec or RFC section in the body, keep PRs small, and disclose AI +assistance. + +## Core Development Principles + +### 1. Maintain Stable Public Interfaces ⚠️ CRITICAL + +The SDK follows [Semantic Versioning](https://semver.org). Before 1.0, minor releases may break API, +but breakage must be deliberate and called out in the PR. + +- Before changing anything `public`, search for its usage in `Sources/`, `Tests/`, `README.md`, and + `Sources/MCPConformance/`. +- Prefer additive changes. Give new initializer parameters and new properties defaults, so existing + call sites still compile. +- To replace an API, keep the old one as a deprecated shim that forwards to the new one: + +```swift +/// Deprecated compatibility factory for older call sites that used `.text("...")`. +@available(*, deprecated, message: "Use .text(text:annotations:_meta:) instead.") +public static func text(_ text: String, metadata: Metadata? = nil) -> Self { + .text(text: text, annotations: nil, _meta: metadata) +} +``` + +- Keep implementation details `internal`. Use `package` only when another target in this package + needs the symbol. +- Wire format is public API too. Changing a `CodingKeys` raw value or an encode/decode rule can + break interoperability with other MCP SDKs. + +### 2. Follow the MCP Specification + +- The SDK implements protocol versions up to `2025-11-25`, listed in `Version.supported` + (`Base/Versioning.swift`). Don't add a newer version until the SDK implements it. Adding a version + also requires updating `VersioningTests`. +- Link each type to its spec section in its doc comment, using the revision the code implements: + +```swift +/// To discover available tools, clients send a `tools/list` request. +/// - SeeAlso: https://modelcontextprotocol.io/specification/2025-11-25/server/tools/#listing-tools +``` + +- Use spec field names as wire keys, including `_meta`. Decode leniently: use `decodeIfPresent` with + sensible defaults for optional fields, so messages from other implementations still parse. +- Gate capability-dependent behavior through the existing `validateServerCapability` / + `validateClientCapability` helpers. Don't add ad-hoc checks. +- Cite RFC sections in comments when implementing HTTP or OAuth behavior, for example + `// RFC 6749 §2.3.1`. + +### 3. Defining Protocol Messages + +A request method is a caseless `enum` that conforms to `Method`. Its `Parameters` and `Result` are +`Hashable, Codable, Sendable` structs: + +❌ **Bad:** + +```swift +public struct ListWidgetsRequest: Codable { + var method = "widgets/list" + var cursor: String? +} +``` + +✅ **Good:** + +```swift +/// - SeeAlso: https://modelcontextprotocol.io/specification/2025-11-25/... +public enum ListWidgets: Method { + public static let name = "widgets/list" + + public struct Parameters: NotRequired, Hashable, Codable, Sendable { + public let cursor: String? + + public init() { self.cursor = nil } + public init(cursor: String) { self.cursor = cursor } + } + + public struct Result: Hashable, Codable, Sendable { + public let widgets: [Widget] + public let nextCursor: String? + public var _meta: Metadata? + + public init(widgets: [Widget], nextCursor: String? = nil, _meta: Metadata? = nil) { ... } + } +} + +public struct WidgetListChangedNotification: Notification { + public static let name: String = "notifications/widgets/list_changed" +} +``` + +- Use `Empty` (the default) or `public typealias Parameters = Empty` for methods without + parameters. Conform `Parameters` to `NotRequired` when clients may omit `params` entirely. +- Expose `_meta` as `public var _meta: Metadata?`, with a `_meta: Metadata? = nil` initializer + parameter. +- Group related types in a namespace `enum` when a feature has many types (see `Sampling`, + `Elicitation`). +- Add a typed convenience method to `Client` or `Server` that callers use instead of building + requests by hand. Requests are sent with `try await send(request).value`. +- Server handlers are registered with `withMethodHandler(_:handler:)`, which replaces any existing + handler for that method. Notification handlers registered with `onNotification(_:handler:)` are + appended. `Server.start` registers default `Initialize` and `Ping` handlers. + +### 4. Concurrency + +- `Client`, `Server`, and every `Transport` are actors. Data types are `Sendable` value types. +- Don't capture `[weak self]` in closures and tasks owned by an actor. Capture what the closure + needs, or rely on the actor's own lifecycle. +- Avoid `@unchecked Sendable`, `nonisolated(unsafe)`, and `@preconcurrency`. Use them only where a + type is provably serialized by an actor, and document why next to the declaration (see + `OAuthAuthorizer`). +- Don't block threads. Don't use `DispatchSemaphore` or busy-waiting in library code. +- New transports conform to `Transport` (`public actor`) with `public nonisolated let logger: Logger`. + They must finish their `receive()` stream on disconnect. + +### 5. Platform Compatibility + +The library builds on every Apple platform and on Linux, including the static Linux SDK. Gate +platform-specific code with `canImport` checks where possible, and follow the existing gates: + +| Concern | Gate | +| ---------------------------------------- | ------------------------------------------------------------------ | +| `Network.framework` (`NetworkTransport`) | `#if canImport(Network)` | +| `URLSession` on Linux | `#if canImport(FoundationNetworking)` | +| SSE streaming (`EventSource`) | `#if !os(Linux)` (the package links it on Apple platforms only) | +| POSIX I/O (`StdioTransport`) | `#if canImport(Darwin) \|\| canImport(Glibc) \|\| canImport(Musl)` | +| `System` vs `SystemPackage` | `#if canImport(System)` | +| PKCE S256 hashing | `#if canImport(CryptoKit)` | + +If a change can affect Linux, call that out in the PR, because CI is the only Linux check most +contributors get. + +### 6. Testing Requirements + +- Every new feature and every bug fix MUST come with tests. A bug fix needs a regression test that + fails without the fix. +- Tests live in `Tests/MCPTests/`, one file per feature area (`ToolTests.swift`, + `HTTPClientTransportTests.swift`, ...). Add to the existing file when there is one. +- Use Swift Testing: a `@Suite("Readable Name") struct`, and `@Test("Readable description")` on each + test. Assert with `#expect` / `#require`, and use `Issue.record` for unreachable paths. + +```swift +@Suite("Tool Tests") +struct ToolTests { + @Test("ListTools result round-trips with _meta") + func testListToolsResultRoundTrip() throws { + let meta = Metadata(additionalFields: ["vendor.example/key": .string("value")]) + let result = ListTools.Result(tools: [], _meta: meta) + let data = try JSONEncoder().encode(result) + let decoded = try JSONDecoder().decode(ListTools.Result.self, from: data) + #expect(decoded == result) + } +} +``` + +- **Wire format**: assert on the encoded JSON (`JSONSerialization`), not only on a decode + round-trip, so a symmetric encode/decode bug can't pass. +- **Client/server behavior**: use a real pair over `InMemoryTransport`: + +```swift +let (clientTransport, serverTransport) = await InMemoryTransport.createConnectedPair() +let server = Server(name: "test", version: "1.0", capabilities: .init(tools: .init())) +await server.withMethodHandler(ListTools.self) { _ in .init(tools: []) } +try await server.start(transport: serverTransport) + +let client = Client(name: "test", version: "1.0") +_ = try await client.connect(transport: clientTransport) +// ... exercise and assert ... +await client.disconnect() +await server.stop() +``` + +Use `MockTransport` (`Tests/MCPTests/Helpers/`) when you need to script exact messages or inspect +what was sent. + +- **Assert real behavior**: test what the SDK did, not state that a mock set itself. +- **No fixed sleeps for synchronization.** Wait for a condition with a deadline: + +❌ **Bad:** + +```swift +try await Task.sleep(for: .milliseconds(500)) +#expect(mockConnection.state == .cancelled) +``` + +✅ **Good:** + +```swift +let deadline = ContinuousClock.now + .seconds(3) +while mockConnection.state != .cancelled && ContinuousClock.now < deadline { + try await Task.sleep(for: .milliseconds(50)) +} +#expect(mockConnection.state == .cancelled) +``` + +- **Shared global state**: the HTTP and OAuth tests share `MockURLProtocol`. Put new tests in a + `.serialized` suite with the `.httpClientTransportSetup` trait, or use the key-isolated helpers + in `OAuthComponentTestHelpers.swift`. Mirror the existing `#if swift(>=6.1)`, + `canImport(FoundationNetworking)`, and `canImport(Network)` gates. +- **Conformance**: for protocol features, add or update a scenario in + `Sources/MCPConformance/Client/main.swift` (`scenarioHandlers`) or `Server/main.swift`. Don't add + entries to `conformance-baseline.yml` to hide a failure you introduced. + +### 7. Documentation + +- Update `README.md` when you add or change user-facing API. Its examples must compile against the + current API. +- Keep doc comments accurate when behavior changes. A stale comment is a bug. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..5f6f98c8 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,113 @@ +# Contributing Guide + +Thank you for your interest in contributing to the MCP Swift SDK! This document provides guidelines +and instructions for contributing. + +If you work with an AI coding agent, also point it at [AGENTS.md](AGENTS.md). That file covers the +technical conventions in this codebase: message definitions, concurrency, platform gates, and test +patterns. + +## AI Policy + +The MCP community allows the use of AI assistant tooling for contributions. Before using AI agents, +please review [the MCP AI Policy](https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/AI_POLICY.md) +to understand the guidelines and responsibilities for such contributions. In short: + +- **Disclose it.** State in the PR description or issue that AI was used, and to what extent. +- **Own it.** You must understand the change and be able to explain it in your own words. When a + maintainer asks a question, the answer should come from you, not be pasted from a chat window. +- **Keep a human in the loop.** Don't let agents open PRs, file issues, or post comments without a + person reviewing the output first. + +## Before You Start + +Open an issue and agree on the approach before writing code for: + +- New public APIs, or changes to existing public API shapes +- Architectural changes, such as the `Transport` protocol or the `Client` / `Server` actors +- Changes that cut across many files or modules +- Behavior not covered by the MCP specification. These need a + [Specification Enhancement Proposal (SEP)](https://modelcontextprotocol.io/seps) + first. + +## Requirements + +- [Swift](https://www.swift.org/install/) (6.1 or later) + - macOS: [Xcode](https://developer.apple.com/xcode/) (16.3 or later), using its bundled toolchain + - Linux: [swiftly](https://www.swift.org/install/linux/) to install Swift, plus `libcurl4-openssl-dev` +- [Node.js](https://nodejs.org/) (22 or later), only needed for the + [MCP conformance tests](https://github.com/modelcontextprotocol/conformance) + +## Development + +You can build the project as follows: + +```bash +swift build +``` + +## Testing + +Use these commands to run tests: + +```bash +swift test # All tests +swift test --filter CompletionTests # One suite +swift package generate-documentation --target MCP --warnings-as-errors # If you changed doc comments +scripts/run-conformance.sh # If you changed protocol behavior or transports +``` + +## Best Practices + +### Commit Messages and PR Descriptions + +- Write the subject in the imperative mood and sentence case, describing the behavior change, for + example `Decode any percent-encoded byte in non-base64 data URLs`. A Conventional Commits prefix + (`fix:`, `test:`, `docs:`, `refactor:`, `chore:`) is recommended. +- Explain **why** in the body: the root cause, the relevant spec or RFC section, and how the change + addresses it. +- Link the issue the PR addresses (`Fixes #123`). +- Disclose AI assistance (see [AI Policy](#ai-policy)). + +### Coding Style + +Follow [the Swift API Design Guidelines](https://www.swift.org/documentation/api-design-guidelines/), +and match the style of the surrounding code. [AGENTS.md](AGENTS.md#core-development-principles) has +more rules and examples for message definitions, concurrency, platform gates, and tests. + +- **Formatting**: use 4-space indentation, as configured in `.vscode/settings.json`. The repository + has no enforced formatter, so don't reformat code you are not otherwise changing. +- **Imports**: core `Base/` files import specific Foundation symbols + (`import struct Foundation.Data`). Other files use `import Foundation`. List `import Logging` + first. +- **No file headers**: source files start with imports, with no license or copyright banner. +- **Doc comments**: document every `public` symbol with `///`, a one-line summary, and DocC + callouts (`- Parameters:`, `- Returns:`, `- Throws:`, `- SeeAlso:`, `- Important:`). Use + ` ``Symbol`` ` links only for symbols that resolve, because CI fails on broken links. Use + single backticks for method names, header names, and other literals. Code samples in doc comments + are not compiled, so check them against the real API. +- **Organization**: separate a type from its extensions with `// MARK: -`, and label sections with + `// MARK: - Name`. Put protocol conformances in separate extensions where the file already does. +- **Codable**: use `private enum CodingKeys: String, CodingKey`. Encode optional fields with + `encodeIfPresent`. +- **Errors**: throw `MCPError` from protocol and transport code: `.invalidParams` for bad input, + `.methodNotFound` for unsupported capabilities, `.invalidRequest` for lifecycle violations, and + `.internalError` otherwise, with a short message. OAuth failures use `OAuthAuthorizationError`. + Don't swallow errors silently. If an error is intentionally ignored, log it and explain why. +- **Logging**: use the transport's swift-log `Logger`. Labels follow `mcp.transport.`, and the + default logger is a no-op. Use `trace` for per-message events, `debug` for lifecycle events, + `warning` for unexpected input, and `error` for failures. Put structured values in `metadata:`. + Never log tokens, secrets, or authorization headers. +- **Security-sensitive code** (`Authorization/`, `HTTPRequestValidation.swift`): validate URLs and + inputs with the existing validators. Keep secrets out of errors and logs, and add tests for each + rejected input. + +## Security + +Report vulnerabilities privately through GitHub Security Advisories, as described in +[SECURITY.md](SECURITY.md). Don't report them in public issues, discussions, or pull requests. + +## License + +By contributing, you agree that new contributions are licensed under Apache 2.0. Existing code +remains under MIT. See [LICENSE](LICENSE). From 9dfe2ce6dd7b02b739f4f7a5c88ac7cd75e75b6b Mon Sep 17 00:00:00 2001 From: Andrey Velichkevich Date: Tue, 3 Nov 2026 00:36:21 +0000 Subject: [PATCH 2/2] Address review comments --- .agents/README.md | 14 ++++++-- .agents/skills/review-pr/SKILL.md | 33 ++++++++++++------- .../skills/review-pr/agents/code-reviewer.md | 2 +- .../review-pr/agents/code-simplifier.md | 29 ++++++++-------- .../review-pr/agents/silent-failure-hunter.md | 18 ++++++---- .../review-pr/hooks/gh-publish-guard.sh | 21 +++++++++--- AGENTS.md | 20 +++++++---- CONTRIBUTING.md | 8 ++--- 8 files changed, 93 insertions(+), 52 deletions(-) diff --git a/.agents/README.md b/.agents/README.md index 33d76f06..0ab21789 100644 --- a/.agents/README.md +++ b/.agents/README.md @@ -61,14 +61,22 @@ instruction. `hooks/gh-publish-guard.sh` is a Claude Code `PreToolUse` hook that enforces: it forces a confirmation prompt before any Bash command that writes to GitHub. The Agent Skills spec has no field for hooks, and Claude Code does not auto-load hook files from -`.claude/hooks/`, so register the hook in `.claude/settings.json`. Run from the repository root: +`.claude/hooks/`, so register the hook yourself. The hook is a personal opt-in, so register it in +`.claude/settings.local.json`. +Run from the repository root: ```bash -[ -f .claude/settings.json ] || echo '{}' > .claude/settings.json +mkdir -p .claude +[ -f .claude/settings.local.json ] || echo '{}' > .claude/settings.local.json jq --arg cmd '"$CLAUDE_PROJECT_DIR"/.agents/skills/review-pr/hooks/gh-publish-guard.sh' ' if any(.hooks.PreToolUse[]?.hooks[]?; .command == $cmd) then . else .hooks.PreToolUse += [{matcher: "Bash", hooks: [{type: "command", command: $cmd}]}] - end' .claude/settings.json > .claude/settings.json.tmp && mv .claude/settings.json.tmp .claude/settings.json + end' .claude/settings.local.json > .claude/settings.local.json.tmp && + mv .claude/settings.local.json.tmp .claude/settings.local.json ``` +The hook asks for confirmation before `gh pr`/`gh issue` write commands, `gh release` changes, and +`gh api` calls that write. That covers an explicit `POST`/`PATCH`/`PUT`/`DELETE`, and `-f`, `-F`, or +`--input` without `--method GET`, because those flags make `gh api` default to `POST`. + Run `/hooks` in Claude Code to verify that it is registered. diff --git a/.agents/skills/review-pr/SKILL.md b/.agents/skills/review-pr/SKILL.md index bd0e0a86..46fe1b2c 100644 --- a/.agents/skills/review-pr/SKILL.md +++ b/.agents/skills/review-pr/SKILL.md @@ -34,7 +34,7 @@ Where `review.json` contains: "body": "Thanks for your work on this! This review was performed using AI tools.", "comments": [ { - "path": "pkg/foo/bar.go", + "path": "Sources/MCP/Server/Tools.swift", "line": 42, "side": "RIGHT", "body": "" @@ -61,10 +61,10 @@ The default and preferred format for every actionable inline comment is a GitHub Example comment body posted via `gh api .../reviews`: ````markdown -Rename to match the suite directory: +Throw `MCPError` so the client receives a JSON-RPC error: ```suggestion -package statusserver + throw MCPError.invalidParams("Unknown tool: \(params.name)") ``` ```` @@ -81,7 +81,13 @@ In these cases, post a small prose comment with a clear action item instead of a ## Review Workflow: 1. **Determine Review Scope** - - Check git status to identify changed files + - **PR link or number given**: review the PR, not the local checkout. Fetch its diff and metadata: + ```bash + gh pr diff --repo / + gh pr view --repo / --json headRefOid,files,title,body + ``` + The local checkout may be on a different branch, so don't use `git diff` for a PR review. + - **No PR given**: review local changes with `git diff` (unstaged) and `git diff --cached` (staged) - Parse arguments to see if user requested specific review aspects - Default: Run all applicable reviews @@ -95,8 +101,7 @@ In these cases, post a small prose comment with a clear action item instead of a - **all** - Run all applicable reviews (default) 3. **Identify Changed Files** - - Run `git diff --name-only` to see modified files - - Check if PR already exists: `gh pr view` + - For a PR, use the `files` list from `gh pr view`. For local changes, run `git diff --name-only` - Identify file types and what reviews apply 4. **Determine Applicable Reviews** @@ -107,14 +112,18 @@ In these cases, post a small prose comment with a clear action item instead of a - **If comments/docs added**: comment-analyzer - **If error handling changed**: silent-failure-hunter - **If types added/modified**: type-design-analyzer - - **After passing review**: code-simplifier (polish and refine) + - **After passing review**: code-simplifier (advisory only: it proposes simplifications as + comments and never edits files) 5. **Launch Review Agents** Each agent's instructions live in [agents/](agents/) next to this file (e.g. `agents/code-reviewer.md`). If an agent with that name is registered in your environment, use it. Otherwise, launch a general-purpose subagent and pass the body of the matching file as its prompt, followed by the - review scope (PR number or changed files). + review scope. + + For a PR review, pass each agent the PR diff from step 1 and tell it to review that diff instead + of the local `git diff`. Agents read files for context only; they must not edit the checkout. **Sequential approach** (one at a time): - Easier to understand and act on @@ -188,7 +197,7 @@ In these cases, post a small prose comment with a clear action item instead of a # Reviews only code comments /review-pr simplify -# Simplifies code after passing review +# Suggests simplifications after passing review ``` **Parallel review:** @@ -232,15 +241,15 @@ In these cases, post a small prose comment with a clear action item instead of a **code-simplifier**: -- Simplifies complex code +- Suggests simplifications for complex code - Improves clarity and readability - Applies project standards -- Preserves functionality +- Advisory only: never edits files ## Tips: - **Run early**: Before creating PR, not after -- **Focus on changes**: Agents analyze git diff by default +- **Focus on changes**: Agents analyze the PR diff when given a PR, and the local `git diff` otherwise - **Address critical first**: Fix high-priority issues before lower priority - **Re-run after fixes**: Verify issues are resolved - **Use specific reviews**: Target specific aspects when you know the concern diff --git a/.agents/skills/review-pr/agents/code-reviewer.md b/.agents/skills/review-pr/agents/code-reviewer.md index a62587bd..9dac054b 100644 --- a/.agents/skills/review-pr/agents/code-reviewer.md +++ b/.agents/skills/review-pr/agents/code-reviewer.md @@ -1,6 +1,6 @@ --- name: code-reviewer -description: Use this agent when you need to review code for adherence to project guidelines, style guides, and best practices. This agent should be used proactively after writing or modifying code, especially before committing changes or creating pull requests. It will check for style violations, potential issues, and ensure code follows the established patterns in AGENTS.md. Also the agent needs to know which files to focus on for the review. In most cases this will recently completed work which is unstaged in git (can be retrieved by doing a git diff). However there can be cases where this is different, make sure to specify this as the agent input when calling the agent. \n\nExamples:\n\nContext: The user has just implemented a new feature with several TypeScript files.\nuser: "I've added the new authentication feature. Can you check if everything looks good?"\nassistant: "I'll use the Task tool to launch the code-reviewer agent to review your recent changes."\n\nSince the user has completed a feature and wants validation, use the code-reviewer agent to ensure the code meets project standards.\n\n\n\nContext: The assistant has just written a new utility function.\nuser: "Please create a function to validate email addresses"\nassistant: "Here's the email validation function:"\n\nassistant: "Now I'll use the Task tool to launch the code-reviewer agent to review this implementation."\n\nProactively use the code-reviewer agent after writing new code to catch issues early.\n\n\n\nContext: The user is about to create a PR.\nuser: "I think I'm ready to create a PR for this feature"\nassistant: "Before creating the PR, I'll use the Task tool to launch the code-reviewer agent to ensure all code meets our standards."\n\nProactively review code before PR creation to avoid review comments and iterations.\n\n +description: Use this agent when you need to review code for adherence to project guidelines, style guides, and best practices. This agent should be used proactively after writing or modifying code, especially before committing changes or creating pull requests. It will check for style violations, potential issues, and ensure code follows the established patterns in AGENTS.md. Also the agent needs to know which files to focus on for the review. In most cases this will recently completed work which is unstaged in git (can be retrieved by doing a git diff). However there can be cases where this is different, make sure to specify this as the agent input when calling the agent. \n\nExamples:\n\nContext: The user has just implemented a new feature with several Swift files.\nuser: "I've added the new authentication feature. Can you check if everything looks good?"\nassistant: "I'll use the Task tool to launch the code-reviewer agent to review your recent changes."\n\nSince the user has completed a feature and wants validation, use the code-reviewer agent to ensure the code meets project standards.\n\n\n\nContext: The assistant has just written a new utility function.\nuser: "Please create a function to validate email addresses"\nassistant: "Here's the email validation function:"\n\nassistant: "Now I'll use the Task tool to launch the code-reviewer agent to review this implementation."\n\nProactively use the code-reviewer agent after writing new code to catch issues early.\n\n\n\nContext: The user is about to create a PR.\nuser: "I think I'm ready to create a PR for this feature"\nassistant: "Before creating the PR, I'll use the Task tool to launch the code-reviewer agent to ensure all code meets our standards."\n\nProactively review code before PR creation to avoid review comments and iterations.\n\n color: green --- diff --git a/.agents/skills/review-pr/agents/code-simplifier.md b/.agents/skills/review-pr/agents/code-simplifier.md index 01bbf58e..13f8b696 100644 --- a/.agents/skills/review-pr/agents/code-simplifier.md +++ b/.agents/skills/review-pr/agents/code-simplifier.md @@ -1,6 +1,6 @@ --- name: code-simplifier -description: Use this agent when code has been written or modified and needs to be simplified for clarity, consistency, and maintainability while preserving all functionality. This agent should be triggered automatically after completing a coding task or writing a logical chunk of code. It simplifies code by following project best practices while retaining all functionality. The agent focuses only on recently modified code unless instructed otherwise.\n\nExamples:\n\n +description: Use this agent when code has been written or modified and you want proposals to simplify it for clarity, consistency, and maintainability while preserving all functionality. It is advisory only - it reports suggested replacements and never edits files. The agent focuses only on the changed code in the review scope unless instructed otherwise.\n\nExamples:\n\n Context: The assistant has just implemented a new feature that adds user authentication to an API endpoint. user: "Please add authentication to the /api/users endpoint" assistant: "I've implemented the authentication for the /api/users endpoint. Here's the code:" @@ -36,15 +36,15 @@ assistant: "Now I'll use the code-simplifier agent to ensure the optimized code You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. -You will analyze recently modified code and apply refinements that: +You will analyze the changed code and propose refinements that: 1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. -2. **Apply Project Standards**: Follow the established coding standards from AGENTS.md and the nearest existing code, including: - - Go conventions and structured error handling - - Python formatting, typing, and logging conventions - - Rust conventions and rustfmt defaults - - Language-appropriate naming conventions +2. **Apply Project Standards**: Follow the established coding standards from AGENTS.md, the Coding Style section of CONTRIBUTING.md, and the nearest existing code, including: + - [Swift API Design Guidelines](https://www.swift.org/documentation/api-design-guidelines/) naming + - Swift Concurrency conventions: actors for `Client`, `Server`, and transports; `Sendable` value types + - Throwing `MCPError` from protocol and transport code, and logging through swift-log `Logger` + - Message definitions as `enum X: Method` with `Hashable, Codable, Sendable` parameters and results 3. **Enhance Clarity**: Simplify code structure by: - Reducing unnecessary complexity and nesting @@ -63,15 +63,14 @@ You will analyze recently modified code and apply refinements that: - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) - Make the code harder to debug or extend -5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. +5. **Focus Scope**: Only propose refinements for code changed in the review scope (the PR diff, or the local diff when no PR is given), unless explicitly instructed to review a broader scope. -Your refinement process: +Your review process: -1. Identify the recently modified code sections +1. Identify the changed code sections in the review scope you were given 2. Analyze for opportunities to improve elegance and consistency -3. Apply project-specific best practices and coding standards -4. Ensure all functionality remains unchanged -5. Verify the refined code is simpler and more maintainable -6. Document only significant changes that affect understanding +3. Check them against project-specific best practices and coding standards +4. Make sure each proposed change keeps all functionality unchanged +5. Propose only refinements that make the code simpler and more maintainable -You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. +IMPORTANT: You are advisory only. Do not edit, create, or delete files, and do not run commands that change the checkout. For each proposal, report the file path and line range, the exact replacement code (so it can be posted as a GitHub `suggestion` block), and a one-line reason. diff --git a/.agents/skills/review-pr/agents/silent-failure-hunter.md b/.agents/skills/review-pr/agents/silent-failure-hunter.md index bd7dc451..d26a4ac1 100644 --- a/.agents/skills/review-pr/agents/silent-failure-hunter.md +++ b/.agents/skills/review-pr/agents/silent-failure-hunter.md @@ -24,12 +24,15 @@ When examining a PR, you will: Systematically locate: -- All try-catch blocks (or try-except in Python, Result types in Rust, etc.) +- All `do`/`catch` blocks, `try?` and `try!` expressions, and `Result` handling - All error callbacks and error event handlers - All conditional branches that handle error states - All fallback logic and default values used on failure - All places where errors are logged but execution continues -- All optional chaining or null coalescing that might hide errors +- All optional chaining (`?.`) or nil-coalescing (`??`) that might hide errors +- Unstructured `Task { }` blocks whose thrown errors are never observed +- Continuations that might never resume, or resume twice +- `AsyncThrowingStream`s that finish without passing the error to the consumer ### 2. Scrutinize Each Error Handler @@ -37,9 +40,9 @@ For every error handling location, ask: **Logging Quality:** -- Is the error logged with appropriate severity (logError for production issues)? +- Is the error logged through the swift-log `Logger` at an appropriate level (`error` for failures, `warning` for unexpected input)? - Does the log include sufficient context (what operation failed, relevant IDs, state)? -- Is there an error ID from constants/errorIds.ts for Sentry tracking? +- Is the error surfaced as the right `MCPError` case (or `OAuthAuthorizationError` in OAuth code), so callers and the JSON-RPC peer get a useful code and message? - Would this log help someone debug the issue 6 months from now? **User Feedback:** @@ -87,7 +90,8 @@ Look for patterns that hide errors: - Empty catch blocks (absolutely forbidden) - Catch blocks that only log and continue -- Returning null/undefined/default values on error without logging +- Returning `nil` or default values on error without logging +- `try?` that discards an error the caller needs - Using optional chaining (?.) to silently skip operations that might fail - Fallback chains that try multiple approaches without explaining why - Retry logic that exhausts attempts without informing the user @@ -99,7 +103,7 @@ Ensure compliance with the project's error handling requirements: - Never silently fail in production code - Always log errors using appropriate logging functions - Include relevant context in error messages -- Use proper error IDs for Sentry tracking +- Map errors to the right `MCPError` case, and never put tokens or secrets in error messages or logs - Propagate errors to appropriate handlers - Never use empty catch blocks - Handle errors explicitly, never suppress them @@ -131,7 +135,7 @@ You are thorough, skeptical, and uncompromising about error handling quality. Yo Be aware of project-specific patterns from AGENTS.md: -- Preserve the existing logging and error-handling conventions of the language and neighboring package. +- Preserve the error-handling and logging conventions of the neighboring file: `MCPError` and swift-log `Logger` (see the Coding Style section of CONTRIBUTING.md). - Never bypass tests or linters. Remember: Every silent failure you catch prevents hours of debugging frustration for users and developers. Be thorough, be skeptical, and never let an error slip through unnoticed. diff --git a/.agents/skills/review-pr/hooks/gh-publish-guard.sh b/.agents/skills/review-pr/hooks/gh-publish-guard.sh index d24dce1a..cc69351d 100755 --- a/.agents/skills/review-pr/hooks/gh-publish-guard.sh +++ b/.agents/skills/review-pr/hooks/gh-publish-guard.sh @@ -13,14 +13,27 @@ fi # `gh` followed by any global options (e.g. `--repo owner/repo`, `-R owner/repo`). gh_re='gh([[:space:]]+-[^[:space:]]+([[:space:]]+[^-[:space:]][^[:space:]]*)?)*[[:space:]]+' -# Commands that write to GitHub. `gh api` is only flagged when it uses a -# write HTTP method, since it defaults to GET (read-only). +# Commands that write to GitHub. write_re="${gh_re}(pr|issue)[[:space:]]+(review|comment|merge|create|edit|close|reopen|ready)" write_re+="|${gh_re}release[[:space:]]+(create|edit|delete)" write_re+="|${gh_re}api[[:space:]].*(--method([[:space:]]+|=)|-X[[:space:]]*)(POST|PATCH|PUT|DELETE)" -# Join shell line continuations so options split across lines are matched. -if printf '%s' "${cmd//$'\\\n'/ }" | grep -Eiq "$write_re"; then +# `gh api` defaults to GET, but switches to POST when it sends parameters +# (-f/--raw-field, -F/--field) or a body (--input), unless GET is set explicitly. +api_body_re="${gh_re}api[[:space:]].*[[:space:]](-[fF][^[:space:]]*|--(raw-)?field([[:space:]]|=)|--input([[:space:]]|=))" +api_get_re="(--method([[:space:]]+|=)|-X[[:space:]]*)GET([[:space:]]|$)" + +# Join shell line continuations so options split across lines are matched, then +# put each chained command (`;`, `&&`, `||`) on its own line so the explicit-GET +# exemption only applies to the command that sets it. +line="${cmd//$'\\\n'/ }" +line="${line//&&/$'\n'}" +line="${line//||/$'\n'}" +line="${line//;/$'\n'}" + +api_writes="$(grep -E "$api_body_re" <<<"$line" | grep -Eiv "$api_get_re" || true)" + +if grep -Eiq "$write_re" <<<"$line" || [[ -n "$api_writes" ]]; then cat <<'JSON' { "hookSpecificOutput": { diff --git a/AGENTS.md b/AGENTS.md index 7fa460b1..914a63dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,8 +11,13 @@ AI agents should: - Follow [CONTRIBUTING.md](CONTRIBUTING.md), including its [Coding Style](CONTRIBUTING.md#coding-style), and the patterns in neighboring files - Build and test locally (see [Commands](#commands)) before proposing a commit -- Read the relevant [MCP specification](https://modelcontextprotocol.io/specification/2026-07-28) - section before changing protocol behavior, and match the spec's wire format exactly +- Read the relevant MCP specification section before changing protocol behavior, and match the + spec's wire format exactly. Use the revision the work targets: + - Changes to existing behavior: [`2025-11-25`](https://modelcontextprotocol.io/specification/2025-11-25), + the latest revision the SDK implements + - Work that explicitly adopts the new spec: [`2026-07-28`](https://modelcontextprotocol.io/specification/2026-07-28) + + Don't mix the two in one change. If the task doesn't say which revision it targets, ask. - Call out every change to public API, and keep source compatibility for existing users Agents must NOT: @@ -22,8 +27,8 @@ Agents must NOT: explicitly asked - Add package dependencies without explicit approval. When approved, update **both** `Package.swift` and `Package@swift-6.0.swift`, and commit the updated `Package.resolved` -- Reformat files they are not otherwise changing. The repository has no formatter configuration, so - project-wide formatting produces noisy diffs +- Commit unrelated formatting changes. `.vscode/settings.json` enables format-on-save, so check the + diff for reformatted lines outside the change and revert them - Commit secrets, credentials, or changes to git config - Post issues, PRs, or review comments to GitHub without a human reviewing them first - Describe security vulnerabilities in public issues, PRs, or commit messages. See @@ -219,8 +224,11 @@ public struct WidgetListChangedNotification: Notification { ### 4. Concurrency - `Client`, `Server`, and every `Transport` are actors. Data types are `Sendable` value types. -- Don't capture `[weak self]` in closures and tasks owned by an actor. Capture what the closure - needs, or rely on the actor's own lifecycle. +- Don't capture `[weak self]` in handlers that `Client` or `Server` register on themselves (for + example `withMethodHandler` or `onNotification` closures in `Server.swift`). The actor owns those + closures, so `self` is never `nil` there. Keep `[weak self]` in callbacks handed to other objects, + such as `NWConnection` handlers and continuation `onTermination` closures in `NetworkTransport`, + where it prevents retain cycles. - Avoid `@unchecked Sendable`, `nonisolated(unsafe)`, and `@preconcurrency`. Use them only where a type is provably serialized by an actor, and document why next to the declaration (see `OAuthAuthorizer`). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5f6f98c8..520c2e2a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,10 +75,10 @@ Follow [the Swift API Design Guidelines](https://www.swift.org/documentation/api and match the style of the surrounding code. [AGENTS.md](AGENTS.md#core-development-principles) has more rules and examples for message definitions, concurrency, platform gates, and tests. -- **Formatting**: use 4-space indentation, as configured in `.vscode/settings.json`. The repository - has no enforced formatter, so don't reformat code you are not otherwise changing. -- **Imports**: core `Base/` files import specific Foundation symbols - (`import struct Foundation.Data`). Other files use `import Foundation`. List `import Logging` +- **Formatting**: use 4-space indentation. `.vscode/settings.json` enables format-on-save, so don't + commit unrelated formatting changes. Check your diff for reformatted lines outside your change. +- **Imports**: match the file you're editing. Some files import specific Foundation symbols + (`import struct Foundation.Data`), and others use `import Foundation`. List `import Logging` first. - **No file headers**: source files start with imports, with no license or copyright banner. - **Doc comments**: document every `public` symbol with `///`, a one-line summary, and DocC