From b08edab41f96d724974f452ce81ea27534be5aab Mon Sep 17 00:00:00 2001 From: Scott McCarty Date: Wed, 25 Mar 2026 20:19:47 -0400 Subject: [PATCH] fix: resolve pre-existing CI failures and gourmand violations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Gourmand CI: switch from cargo install to container image, add .gourmand-cache/ to .gitignore 2. Constitution: add missing Section III (Containerfile Conventions), renumber sections to match parent profile (I-IX) 3. Container Security Scan: use docker build on GHA runners so Trivy can find the image 4. Gourmand violations: remove verbose comments, rename generic vars, extract magic numbers to constants, add C901 + pylint thresholds, fix type: ignore, clean up exceptions file (40 → 0 violations) Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/ci.yml | 24 +--------- .github/workflows/security.yml | 2 +- .gitignore | 3 ++ .specify/memory/constitution.md | 26 ++++++++--- gourmand-exceptions.toml | 30 +------------ pyproject.toml | 6 ++- src/mcp_slack_crunchtools/__init__.py | 24 +--------- src/mcp_slack_crunchtools/client.py | 50 +++++---------------- src/mcp_slack_crunchtools/config.py | 41 ++++++----------- src/mcp_slack_crunchtools/errors.py | 8 +++- src/mcp_slack_crunchtools/models.py | 14 +----- src/mcp_slack_crunchtools/server.py | 21 +-------- src/mcp_slack_crunchtools/tools/__init__.py | 11 +---- tests/test_tools.py | 7 +-- 14 files changed, 67 insertions(+), 200 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c5738b7..4488952 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,30 +41,10 @@ jobs: gourmand: name: Code Quality (Gourmand) runs-on: ubuntu-latest - + container: + image: quay.io/crunchtools/gourmand:latest steps: - uses: actions/checkout@v4 - - - name: Install Rust toolchain - uses: dtolnay/rust-toolchain@stable - - - name: Cache Gourmand binary - uses: actions/cache@v4 - with: - path: | - ~/.cargo/bin/ - ~/.cargo/registry/ - ~/.cargo/git/ - key: ${{ runner.os }}-cargo-gourmand-${{ hashFiles('gourmand.toml') }} - restore-keys: | - ${{ runner.os }}-cargo-gourmand- - - - name: Install Gourmand - run: | - if ! command -v gourmand &> /dev/null; then - cargo install --git https://codeberg.org/mattdm/gourmand.git - fi - - name: Run Gourmand run: gourmand --full . diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 71db22c..5a73477 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -105,7 +105,7 @@ jobs: - name: Build container image run: | - podman build -t mcp-slack:scan . + docker build -f Containerfile -t mcp-slack:scan . - name: Run Trivy vulnerability scanner uses: aquasecurity/trivy-action@master diff --git a/.gitignore b/.gitignore index f304cbf..5f0de58 100644 --- a/.gitignore +++ b/.gitignore @@ -77,6 +77,9 @@ dmypy.json # ruff .ruff_cache/ +# gourmand +.gourmand-cache/ + # uv .uv/ uv.lock diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md index 58aa1d9..f3cbfe2 100644 --- a/.specify/memory/constitution.md +++ b/.specify/memory/constitution.md @@ -1,6 +1,6 @@ # mcp-slack-crunchtools Constitution -> **Version:** 1.1.0 +> **Version:** 1.2.0 > **Ratified:** 2026-03-25 > **Status:** Active > **Inherits:** [crunchtools/constitution](https://github.com/crunchtools/constitution) v1.0.0 @@ -127,7 +127,18 @@ All code MUST pass Gourmand checks before merge. Zero violations required. --- -## III. Testing Standards +## III. Containerfile Conventions + +The container uses a multi-stage Hummingbird FIPS build: + +1. **Builder stage** (`quay.io/hummingbird/python:latest-fips-builder`) — has shell, dnf, build tools. Creates a Python venv and installs all dependencies. +2. **Runtime stage** (`quay.io/hummingbird/python:latest-fips`) — distroless, no shell, no package manager. The venv is copied from the builder. No `RUN` commands in this stage. + +Builder and runtime MUST be from the same Hummingbird ecosystem. Never mix with UBI, Fedora, or Alpine base images. + +--- + +## IV. Testing Standards ### Mocked API Tests (MANDATORY) @@ -156,7 +167,7 @@ Every Pydantic model in `models.py` MUST have tests in `test_validation.py`: --- -## IV. Gourmand (AI Slop Detection) +## V. Gourmand (AI Slop Detection) All code MUST pass `gourmand --full .` with **zero violations** before merge. Gourmand is a CI gate in GitHub Actions. @@ -180,7 +191,7 @@ Unacceptable reasons: --- -## V. Code Quality Gates +## VI. Code Quality Gates Every code change must pass through these gates in order: @@ -203,7 +214,7 @@ Every code change must pass through these gates in order: --- -## VI. Naming Conventions +## VII. Naming Conventions | Context | Name | |---------|------| @@ -218,7 +229,7 @@ Every code change must pass through these gates in order: --- -## VII. Development Workflow +## VIII. Development Workflow ### Adding a New Tool @@ -240,7 +251,7 @@ Every code change must pass through these gates in order: --- -## VIII. Governance +## IX. Governance ### Amendment Process @@ -255,3 +266,4 @@ Every code change must pass through these gates in order: |---------|------|---------| | 1.0.0 | 2026-03-25 | Initial constitution | | 1.1.0 | 2026-03-25 | Switch to Hummingbird distroless FIPS with multi-stage venv build pattern | +| 1.2.0 | 2026-03-25 | Add Section III (Containerfile Conventions), renumber to match parent profile | diff --git a/gourmand-exceptions.toml b/gourmand-exceptions.toml index 446eb6e..b8e5372 100644 --- a/gourmand-exceptions.toml +++ b/gourmand-exceptions.toml @@ -1,5 +1,4 @@ # Gourmand Exceptions -# Documented exceptions for files that legitimately trigger checks # Summary litter — CLAUDE.md is a standard Claude Code project instructions file [[exceptions]] @@ -7,35 +6,8 @@ check = "summary_litter" path = "CLAUDE.md" justification = "Claude Code project instructions for AI assistant orientation" -# Primitive obsession — HTTP status codes and response limits are standard API patterns -[[exceptions]] -check = "primitive_obsession" -path = "src/mcp_slack_crunchtools/client.py" -justification = "HTTP status codes (429) and response size limit (10MB) are standard well-known constants" - +# Primitive obsession — port 8005 is the allocated MCP server port [[exceptions]] check = "primitive_obsession" path = "src/mcp_slack_crunchtools/__init__.py" justification = "Port 8005 is the allocated MCP server port for mcp-slack" - -# Implicit state machine — error handling dispatches on error codes, not a state machine -[[exceptions]] -check = "implicit_state_machine" -path = "src/mcp_slack_crunchtools/client.py" -justification = "Error handling dispatches on Slack error codes — standard pattern, not a state machine" - -# Implicit state machine — optional parameter builders use simple if-guards -[[exceptions]] -check = "implicit_state_machine" -path = "src/mcp_slack_crunchtools/tools/channels.py" -justification = "Optional API parameter builders use simple if-guards to add query params, not state machines" - -[[exceptions]] -check = "implicit_state_machine" -path = "src/mcp_slack_crunchtools/tools/messages.py" -justification = "Optional API parameter builders use simple if-guards to add query params, not state machines" - -[[exceptions]] -check = "implicit_state_machine" -path = "src/mcp_slack_crunchtools/tools/users.py" -justification = "Optional API parameter builders use simple if-guards to add query params, not state machines" diff --git a/pyproject.toml b/pyproject.toml index 11ca23f..c8b9e58 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -47,7 +47,7 @@ line-length = 100 target-version = "py310" [tool.ruff.lint] -select = ["E", "F", "I", "N", "W", "UP", "S", "B", "C4", "DTZ", "T10", "ISC", "PIE", "PT", "RET", "SIM", "TID", "TCH", "ARG", "PLC", "PLE", "PLR", "PLW", "TRY", "RUF"] +select = ["E", "F", "I", "N", "W", "UP", "S", "B", "C4", "C901", "DTZ", "T10", "ISC", "PIE", "PT", "RET", "SIM", "TID", "TCH", "ARG", "PLC", "PLE", "PLR", "PLW", "TRY", "RUF"] ignore = [ "S101", # assert allowed in tests "TRY003", # long exception messages ok @@ -61,6 +61,10 @@ ignore = [ "RUF022", # __all__ sorted by category, not alphabetically ] +[tool.ruff.lint.pylint] +max-branches = 10 +max-statements = 50 + [tool.ruff.lint.per-file-ignores] "tests/*" = ["S105", "PLC0415", "PLR2004"] diff --git a/src/mcp_slack_crunchtools/__init__.py b/src/mcp_slack_crunchtools/__init__.py index b128d80..240c9b7 100644 --- a/src/mcp_slack_crunchtools/__init__.py +++ b/src/mcp_slack_crunchtools/__init__.py @@ -1,26 +1,4 @@ -"""MCP Slack CrunchTools - Secure read-only MCP server for Slack. - -A security-focused read-only MCP server for Slack workspaces. - -Usage: - # Run directly - mcp-slack-crunchtools - - # Or with Python module - python -m mcp_slack_crunchtools - - # With uvx - uvx mcp-slack-crunchtools - -Environment Variables (one of these auth modes): - SLACK_USER_TOKEN: Slack User OAuth Token (xoxp-). - SLACK_COOKIE_TOKEN + SLACK_COOKIE_D: Browser cookie auth (xoxc- + xoxd-). - -Example with Claude Code: - claude mcp add mcp-slack-crunchtools \\ - --env SLACK_USER_TOKEN=xoxp-your-token \\ - -- uvx mcp-slack-crunchtools -""" +"""MCP Slack CrunchTools - Secure read-only MCP server for Slack.""" import argparse diff --git a/src/mcp_slack_crunchtools/client.py b/src/mcp_slack_crunchtools/client.py index 11ecdf8..aa6c590 100644 --- a/src/mcp_slack_crunchtools/client.py +++ b/src/mcp_slack_crunchtools/client.py @@ -1,8 +1,5 @@ """Slack API client with security hardening. -This module provides a secure async HTTP client for the Slack Web API. -All requests go through this client to ensure consistent security practices. - Slack-specific notes: - Slack uses POST for all API methods, even reads - Responses use {"ok": false, "error": "code"} instead of HTTP status codes @@ -26,13 +23,9 @@ logger = logging.getLogger(__name__) -# Response size limit to prevent memory exhaustion (10MB) MAX_RESPONSE_SIZE = 10 * 1024 * 1024 - -# Request timeout in seconds REQUEST_TIMEOUT = 30.0 -# Map Slack error codes to specific exception types _ERROR_MAP: dict[str, type[Exception]] = { "not_authed": PermissionDeniedError, "invalid_auth": PermissionDeniedError, @@ -59,18 +52,15 @@ class SlackClient: """ def __init__(self) -> None: - """Initialize the Slack client.""" self._config = get_config() self._client: httpx.AsyncClient | None = None async def _get_client(self) -> httpx.AsyncClient: - """Get or create the async HTTP client.""" if self._client is None: headers: dict[str, str] = { "Authorization": f"Bearer {self._config.token}", "Content-Type": "application/x-www-form-urlencoded", } - # Cookie auth (xoxc-) requires the d cookie alongside the token if self._config.cookie_d is not None: headers["Cookie"] = f"d={self._config.cookie_d}" self._client = httpx.AsyncClient( @@ -82,7 +72,6 @@ async def _get_client(self) -> httpx.AsyncClient: return self._client async def close(self) -> None: - """Close the HTTP client.""" if self._client is not None: await self._client.aclose() self._client = None @@ -112,57 +101,41 @@ async def api_call( MissingScopeError: When OAuth scope is missing """ client = await self._get_client() - - # Log request (without sensitive data) logger.debug("Slack API call: %s", method) - # Build form data, filtering out None values - data: dict[str, Any] = {} + form_params: dict[str, Any] = {} if params: for key, value in params.items(): if value is not None: - data[key] = value + form_params[key] = value try: - response = await client.post(f"/{method}", data=data) + response = await client.post(f"/{method}", data=form_params) except httpx.TimeoutException as e: raise SlackApiError("timeout", f"Request timeout: {e}") from e except httpx.RequestError as e: raise SlackApiError("request_error", f"Request failed: {e}") from e - # Handle HTTP-level rate limiting (429) if response.status_code == 429: retry_after = response.headers.get("Retry-After") raise RateLimitError(int(retry_after) if retry_after else None) - # Check response size before parsing content_length = response.headers.get("content-length") if content_length and int(content_length) > MAX_RESPONSE_SIZE: raise SlackApiError("response_too_large", "Response too large") - # Parse response try: - result = response.json() + api_response: dict[str, Any] = response.json() except ValueError as e: raise SlackApiError("invalid_json", f"Invalid JSON response: {e}") from e - # Check Slack's ok field - if not result.get("ok"): - error_code = result.get("error", "unknown_error") - self._handle_slack_error(error_code, result) - - return result # type: ignore[no-any-return] + if not api_response.get("ok"): + error_code = api_response.get("error", "unknown_error") + self._handle_slack_error(error_code, api_response) - def _handle_slack_error(self, error_code: str, data: dict[str, Any]) -> None: - """Handle Slack API error responses. + return api_response - Args: - error_code: Slack error code string - data: Full response data - - Raises: - Various UserError subclasses based on error code - """ + def _handle_slack_error(self, error_code: str, response_body: dict[str, Any]) -> None: error_class = _ERROR_MAP.get(error_code) if error_class is PermissionDeniedError: @@ -172,16 +145,15 @@ def _handle_slack_error(self, error_code: str, data: dict[str, Any]) -> None: if error_class is UserNotFoundError: raise UserNotFoundError(error_code) if error_class is RateLimitError: - retry_after = data.get("retry_after") + retry_after = response_body.get("retry_after") raise RateLimitError(int(retry_after) if retry_after else None) if error_class is MissingScopeError: - needed = data.get("needed", error_code) + needed = response_body.get("needed", error_code) raise MissingScopeError(str(needed)) raise SlackApiError(error_code) -# Global client instance _client: SlackClient | None = None diff --git a/src/mcp_slack_crunchtools/config.py b/src/mcp_slack_crunchtools/config.py index eab319a..34486ec 100644 --- a/src/mcp_slack_crunchtools/config.py +++ b/src/mcp_slack_crunchtools/config.py @@ -1,12 +1,4 @@ -"""Secure configuration handling. - -This module handles all configuration including the sensitive Slack tokens. -Tokens are stored as SecretStr to prevent accidental logging. - -Supports two authentication modes: - 1. OAuth App Token: SLACK_USER_TOKEN (xoxp-) - requires workspace admin approval - 2. Cookie Auth: SLACK_COOKIE_TOKEN (xoxc-) + SLACK_COOKIE_D (xoxd-) - uses browser session -""" +"""Secure configuration handling with SecretStr credential storage.""" import logging import os @@ -27,11 +19,6 @@ class Config: """ def __init__(self) -> None: - """Initialize configuration from environment variables. - - Raises: - ConfigurationError: If required environment variables are missing. - """ oauth_token = os.environ.get("SLACK_USER_TOKEN") cookie_token = os.environ.get("SLACK_COOKIE_TOKEN") cookie_d = os.environ.get("SLACK_COOKIE_D") @@ -41,22 +28,26 @@ def __init__(self) -> None: self._cookie_d: SecretStr | None = None self._auth_mode = "oauth" logger.info("Configuration loaded (OAuth token mode)") - elif cookie_token and cookie_d: + return + + if cookie_token and cookie_d: self._token = SecretStr(cookie_token) self._cookie_d = SecretStr(cookie_d) self._auth_mode = "cookie" logger.info("Configuration loaded (cookie auth mode)") - elif cookie_token and not cookie_d: + return + + if cookie_token: raise ConfigurationError( "SLACK_COOKIE_D environment variable required when using SLACK_COOKIE_TOKEN. " "Extract the 'd' cookie value from your browser's Slack session." ) - else: - raise ConfigurationError( - "Slack authentication required. Set one of:\n" - " - SLACK_USER_TOKEN (xoxp-...) for OAuth app token\n" - " - SLACK_COOKIE_TOKEN (xoxc-...) + SLACK_COOKIE_D (xoxd-...) for cookie auth" - ) + + raise ConfigurationError( + "Slack authentication required. Set one of:\n" + " - SLACK_USER_TOKEN (xoxp-...) for OAuth app token\n" + " - SLACK_COOKIE_TOKEN (xoxc-...) + SLACK_COOKIE_D (xoxd-...) for cookie auth" + ) @property def token(self) -> str: @@ -77,10 +68,7 @@ def auth_mode(self) -> str: @property def api_base_url(self) -> str: - """Hardcoded Slack API base URL. - - This is intentionally not configurable to prevent SSRF attacks. - """ + """Hardcoded to prevent SSRF — not configurable by design.""" return "https://slack.com/api" def __repr__(self) -> str: @@ -92,7 +80,6 @@ def __str__(self) -> str: return f"Config(mode={self._auth_mode}, token=***)" -# Global configuration instance - initialized on first import _config: Config | None = None diff --git a/src/mcp_slack_crunchtools/errors.py b/src/mcp_slack_crunchtools/errors.py index 5f23903..62bf999 100644 --- a/src/mcp_slack_crunchtools/errors.py +++ b/src/mcp_slack_crunchtools/errors.py @@ -6,6 +6,8 @@ import os +MAX_SAFE_ID_LENGTH = 20 + class UserError(Exception): """Base class for safe errors that can be shown to users. @@ -66,7 +68,8 @@ class ChannelNotFoundError(UserError): """Channel not found or not accessible.""" def __init__(self, identifier: str) -> None: - safe_id = identifier[:20] + "..." if len(identifier) > 20 else identifier + truncated = len(identifier) > MAX_SAFE_ID_LENGTH + safe_id = identifier[:MAX_SAFE_ID_LENGTH] + "..." if truncated else identifier super().__init__(f"Channel not found or not accessible: {safe_id}") @@ -74,7 +77,8 @@ class UserNotFoundError(UserError): """User not found.""" def __init__(self, identifier: str) -> None: - safe_id = identifier[:20] + "..." if len(identifier) > 20 else identifier + truncated = len(identifier) > MAX_SAFE_ID_LENGTH + safe_id = identifier[:MAX_SAFE_ID_LENGTH] + "..." if truncated else identifier super().__init__(f"User not found: {safe_id}") diff --git a/src/mcp_slack_crunchtools/models.py b/src/mcp_slack_crunchtools/models.py index f597ade..0fc7893 100644 --- a/src/mcp_slack_crunchtools/models.py +++ b/src/mcp_slack_crunchtools/models.py @@ -1,19 +1,7 @@ -"""Pydantic models for input validation. - -All tool inputs are validated through these models to prevent injection attacks -and ensure data integrity before making API calls. - -Slack ID formats: -- Channel: C + 8+ alphanumeric (public), G + 8+ (private/group), D + 8+ (DM) -- User: U + 8+ alphanumeric, W + 8+ (enterprise grid), B + 8+ (bot) -- Team: T + 8+ alphanumeric -- File: F + 8+ alphanumeric -- Timestamp: digits.digits (e.g., 1234567890.123456) -""" +"""Input validation for Slack resource identifiers.""" import re -# Regex patterns for Slack ID validation CHANNEL_ID_PATTERN = re.compile(r"^[CDG][A-Z0-9]{8,}$") USER_ID_PATTERN = re.compile(r"^[UWB][A-Z0-9]{8,}$") TEAM_ID_PATTERN = re.compile(r"^T[A-Z0-9]{8,}$") diff --git a/src/mcp_slack_crunchtools/server.py b/src/mcp_slack_crunchtools/server.py index 5a5b585..232197a 100644 --- a/src/mcp_slack_crunchtools/server.py +++ b/src/mcp_slack_crunchtools/server.py @@ -1,7 +1,4 @@ -"""FastMCP server setup for Slack MCP. - -This module creates and configures the MCP server with all read-only tools. -""" +"""FastMCP server with read-only Slack tools.""" import logging from typing import Any @@ -28,7 +25,6 @@ logger = logging.getLogger(__name__) -# Create the FastMCP server mcp = FastMCP( name="mcp-slack-crunchtools", version="0.1.2", @@ -36,9 +32,6 @@ ) -# Auth tool - - @mcp.tool() async def slack_auth_test() -> dict[str, Any]: """Test Slack authentication and get info about the token owner. @@ -49,9 +42,6 @@ async def slack_auth_test() -> dict[str, Any]: return await auth_test() -# Channel tools - - @mcp.tool() async def slack_list_channels( types: str = "public_channel,private_channel", @@ -174,9 +164,6 @@ async def slack_list_channel_members( return await list_channel_members(channel_id=channel_id, limit=limit, cursor=cursor) -# Message tools - - @mcp.tool() async def slack_search_messages( query: str, @@ -262,9 +249,6 @@ async def slack_list_stars( return await list_stars(count=count, page=page, cursor=cursor) -# User tools - - @mcp.tool() async def slack_get_user_info(user_id: str) -> dict[str, Any]: """Get information about a Slack user. @@ -312,9 +296,6 @@ async def slack_get_user_profile( return await get_user_profile(user_id=user_id, include_labels=include_labels) -# File tools - - @mcp.tool() async def slack_list_files( channel_id: str | None = None, diff --git a/src/mcp_slack_crunchtools/tools/__init__.py b/src/mcp_slack_crunchtools/tools/__init__.py index 4c298d3..4c88b6c 100644 --- a/src/mcp_slack_crunchtools/tools/__init__.py +++ b/src/mcp_slack_crunchtools/tools/__init__.py @@ -1,8 +1,4 @@ -"""Slack MCP tools. - -This package contains all the MCP tool implementations for Slack operations. -All tools are read-only. -""" +"""Read-only Slack MCP tool implementations.""" from .channels import ( get_channel_history, @@ -27,24 +23,19 @@ ) __all__ = [ - # Auth "auth_test", - # Channels "list_channels", "get_channel_info", "get_channel_history", "get_thread_replies", "list_channel_members", - # Messages "search_messages", "get_reactions", "list_reactions", "list_stars", - # Users "get_user_info", "list_users", "get_user_profile", - # Files "list_files", "get_file_info", ] diff --git a/tests/test_tools.py b/tests/test_tools.py index 6e6a68d..42faa1d 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -1,8 +1,4 @@ -"""Tests for MCP tools. - -These tests verify tool behavior without making actual API calls. -Integration tests with a real Slack workspace should be run separately. -""" +"""Tests for MCP tool registration and imports.""" import os @@ -39,7 +35,6 @@ def test_imports(self) -> None: search_messages, ) - # Verify all functions are callable assert callable(auth_test) assert callable(list_channels) assert callable(get_channel_info)