fix: address swarm review findings (C1, C3, C4, R5, R6)

Critical bugs fixed:
- C1: Add null guard for response.data in streaming.py
  SDK can return None when recursing into tool_result fragments
- C3: Apply redact_sensitive_text() to extracted context values
  Prevents credential leakage in error messages
- C4: Change log_response_text default to False (secure default)
  Aligns Python mechanism with YAML policy per Three-Medium

Improvements:
- R5: Add E2E tool capture happy-path tests (TestE2EToolCaptureHappyPath)
  Covers complete flow from SDK event to accumulated buffer
- R6: Fix membrane import in models.py
  Use sdk_adapter namespace, not internal model_translation module

Test results: 980 passed, 94% coverage (Windows + WSL)

Author: HDMowri

amplifier_module_provider_github_copilot/__init__.py

@@ -55,15 +55,28 @@
 # Using importlib.metadata avoids importing the SDK itself at module load time.
 # Contract: sdk-boundary.md MUST:5
 #
-# TESTING: Set SKIP_SDK_CHECK=1 to allow test imports without SDK installed.
-# Tests use pytest.importorskip() and skip markers to handle SDK availability.
+# P1-6 Security Fix: SDK check bypass only allowed in pytest context.
+# The env var alone is not sufficient - pytest must be loaded in sys.modules.
+# This prevents production misuse while preserving test functionality.
 import asyncio
 import os as _os
+import sys as _sys
 import threading
 from importlib.metadata import PackageNotFoundError as _PkgNotFoundError
 from importlib.metadata import version as _pkg_version
 
-if not _os.environ.get("SKIP_SDK_CHECK"):
+
+def _is_pytest_running() -> bool:
+    """Check if pytest is running (for test-only SDK check bypass)."""
+    return "pytest" in _sys.modules
+
+
+# Only skip SDK check if BOTH conditions are met:
+# 1. SKIP_SDK_CHECK env var is set
+# 2. pytest is actually running (prevents production misuse)
+_skip_sdk_check = _os.environ.get("SKIP_SDK_CHECK") and _is_pytest_running()
+
+if not _skip_sdk_check:
     try:
         _pkg_version("github-copilot-sdk")
     except _PkgNotFoundError as _e:
@@ -72,15 +85,17 @@
             "Install with:  pip install 'github-copilot-sdk>=0.2.0,<0.3.0'"
         ) from _e
 
-from collections.abc import Awaitable, Callable
-from typing import Any
+# E402: These imports are intentionally after SDK check - we verify SDK
+# installation before importing modules that depend on it (Three-Medium).
+from collections.abc import Awaitable, Callable  # noqa: E402
+from typing import Any  # noqa: E402
 
-from amplifier_core import ModelInfo, ModuleCoordinator, ProviderInfo
+from amplifier_core import ModelInfo, ModuleCoordinator, ProviderInfo  # noqa: E402
 
-from .provider import GitHubCopilotProvider
+from .provider import GitHubCopilotProvider  # noqa: E402
 
 # Contract: sdk-boundary:Membrane:MUST:1 — import from sdk_adapter package, not submodules
-from .sdk_adapter import CopilotClientWrapper
+from .sdk_adapter import CopilotClientWrapper  # noqa: E402
 
 __version__ = "2.0.0"
 

amplifier_module_provider_github_copilot/error_translation.py

@@ -454,6 +454,9 @@ def translate_sdk_error(
 
             # Extract context from message (use original for pattern matching)
             context = _extract_context(original_message, mapping.context_extraction)
+            # P0 Fix (C3): Redact extracted context values - they come from the
+            # original (unredacted) message and may contain secrets like tokens.
+            context = {k: redact_sensitive_text(v) for k, v in context.items()}
             # Use redacted message for output, append context
             message = safe_message + _format_context_suffix(context)
 

amplifier_module_provider_github_copilot/fake_tool_detection.py

@@ -33,7 +33,9 @@ class LoggingConfig:
     """Logging configuration for fake tool call detection."""
 
     log_matched_pattern: bool = True
-    log_response_text: bool = True
+    # P1 Fix (C4): Secure default matches YAML (false). If YAML fails to load,
+    # we fail closed (don't log potentially sensitive LLM response text).
+    log_response_text: bool = False
     log_response_text_limit: int = 500
     log_tool_calls: bool = True
     log_correction_message: bool = True

amplifier_module_provider_github_copilot/models.py

@@ -66,11 +66,11 @@ def __init__(self, message: str, *, provider: str = "github-copilot") -> None:
 )
 
 # =============================================================================
-# Re-export from sdk_adapter for backward compatibility
-# The actual implementation is now in sdk_adapter/model_translation.py
+# Re-export from sdk_adapter membrane (NOT direct module import)
+# R6 Fix: Import via membrane __init__.py, not bypassing to model_translation.py
 # Contract: sdk-boundary:ModelDiscovery:MUST:2
 # =============================================================================
-from .sdk_adapter.model_translation import (  # noqa: E402
+from .sdk_adapter import (  # noqa: E402
     CopilotModelInfo,
     sdk_model_to_copilot_model,
 )

amplifier_module_provider_github_copilot/sdk_adapter/_imports.py

@@ -14,6 +14,7 @@
 from __future__ import annotations
 
 import os
+import sys
 from typing import Any
 
 # Re-export SDK-independent utilities for backward compatibility.
@@ -24,10 +25,20 @@
 # SDK imports - THE ONLY PLACE IN THE CODEBASE where SDK is imported
 # =============================================================================
 
-# TESTING: Set SKIP_SDK_CHECK=1 to allow imports without SDK installed.
-# Tests use pytest.importorskip() and skip markers to handle SDK availability.
-# This matches the pattern in __init__.py for consistent test behavior.
-_SKIP_SDK_CHECK = os.environ.get("SKIP_SDK_CHECK")
+
+def _is_pytest_running() -> bool:
+    """Check if pytest is running (for test-only SDK bypass).
+
+    P1-6 Security Fix: SDK bypass only allowed when pytest is actually running.
+    This prevents production misuse while preserving test functionality.
+    """
+    return "pytest" in sys.modules
+
+
+# Only skip SDK imports if BOTH conditions are met:
+# 1. SKIP_SDK_CHECK env var is set
+# 2. pytest is actually running (prevents production misuse)
+_SKIP_SDK_CHECK = os.environ.get("SKIP_SDK_CHECK") and _is_pytest_running()
 
 # Guard against import failures - fail fast with clear error
 # Unless SKIP_SDK_CHECK is set (for testing without SDK)

amplifier_module_provider_github_copilot/streaming.py

@@ -596,11 +596,13 @@ def extract_response_content(response: Any, _depth: int = 0) -> str:
     if _depth > _MAX_EXTRACTION_DEPTH:
         return ""
 
-    # Check for .data wrapper first (response.data -> Data object)
-    if hasattr(response, "data"):
+    # P0 Fix (C1): Added null guard to .data check. Previous code recursed into
+    # .data even when None, returning "" silently when .content was valid.
+    # SDK design: .data wraps a nested Data object with .content - unwrap first.
+    if hasattr(response, "data") and response.data is not None:
         return extract_response_content(response.data, _depth + 1)
 
-    # Check for Data object with .content attribute
+    # Fall back to direct .content if .data doesn't exist or is None
     if hasattr(response, "content"):
         content = response.content  # type: ignore[union-attr]
         return str(content) if content is not None else ""

contracts/deny-destroy.md

@@ -1,11 +1,12 @@
 # Contract: Deny + Destroy Pattern
 
 ## Version
-- **Current:** 1.2 (ToolSuppression-Corrected)
+- **Current:** 1.3 (NoTools-Clarified)
 - **Module Reference:** amplifier_module_provider_github_copilot/sdk_adapter/client.py
 - **Status:** Non-Negotiable Constraint
 - **History:**
-  - **1.2** — Fixed ToolSuppression: removed incorrect `available_tools=[]` claim, documented `overrides_built_in_tool=True` approach
+  - **1.3** — Clarified ToolSuppression: `available_tools=[]` required when NO tools provided, allowlist when tools provided
+  - **1.2** — Fixed ToolSuppression for WITH-tools case: documented `overrides_built_in_tool=True` approach
   - **1.1** — Fixed path from `modules/provider-core/session_factory.py` to actual location
 
 ---
@@ -123,8 +124,9 @@ From GOLDEN_VISION_V2.md:
 
 | Anchor | Clause |
 |--------|--------|
-| `deny-destroy:ToolSuppression:MUST:1` | available_tools set to Amplifier tool names (allowlist) |
+| `deny-destroy:ToolSuppression:MUST:1` | available_tools set to Amplifier tool names (allowlist) when tools provided |
 | `deny-destroy:ToolSuppression:MUST:2` | overrides_built_in_tool=True (see sdk-boundary:ToolForwarding:MUST:2) |
+| `deny-destroy:ToolSuppression:MUST:3` | available_tools=[] when NO Amplifier tools provided (blocks SDK built-ins) |
 
 ### Allowlist
 

tests/conftest.py

@@ -4,18 +4,27 @@
 
 All fixtures are typed for pyright strict mode compliance.
 Windows event loop policy configured for asyncio subprocess compatibility.
+
+P1-6 Security Fix: SKIP_SDK_CHECK is now guarded by _is_pytest_running() in
+__init__.py and _imports.py, making it safe to use in tests while preventing
+production misuse.
 """
 
 from __future__ import annotations
 
 import os
 import sys
+from collections.abc import Generator
 from typing import TYPE_CHECKING, Any
 
 import pytest
 
+# P1-6 Security: SKIP_SDK_CHECK is guarded by _is_pytest_running() in production code.
+# This allows tests to run without the SDK while preventing production misuse.
+os.environ["SKIP_SDK_CHECK"] = "1"
+
 # Store original function before any test patches run
-import amplifier_module_provider_github_copilot.models as _models_module
+import amplifier_module_provider_github_copilot.models as _models_module  # noqa: E402
 
 _original_fetch_and_map_models = _models_module.fetch_and_map_models
 
@@ -27,9 +36,6 @@
 
     asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
 
-# Allow test imports without SDK installed - tests use skip markers for SDK tests
-os.environ["SKIP_SDK_CHECK"] = "1"
-
 # P1-6 Security Fix: Clear GitHub token env vars for non-live tests.
 # This prevents the fail-closed security behavior from triggering when
 # SubprocessConfig is None (due to SKIP_SDK_CHECK=1) and a token exists.
@@ -219,9 +225,6 @@ def sdk_module() -> Any:
     return pytest.importorskip("copilot", reason="github-copilot-sdk not installed")
 
 
-from collections.abc import Generator
-
-
 @pytest.fixture
 def restore_github_tokens() -> Generator[None, None, None]:
     """Restore saved GitHub tokens for live tests.