From 026b706b3cd9c9dee977a1140bd483870999bc87 Mon Sep 17 00:00:00 2001 From: Imran Siddique Date: Thu, 6 Aug 2026 09:09:33 -0700 Subject: [PATCH 1/2] fix(inspection): stage 3 content classification was dead when AGT is installed `_classify_sensitivity` called `_agt_redactor.find_credentials(response_text)`. That method has never existed on agt-core's `CredentialRedactor`. Verified against the pinned 4.1.0 wheel and against every commit in AGT's history: the real API is `find_matches` (secrets) and `find_pii_matches` (PII spans). The call raised `AttributeError` straight into a bare `except Exception: pass`, and because the local `_PII_PATTERNS` sweep sat in the `else` branch of the `_AGT_AVAILABLE and _agt_redactor is not None` check, it never ran either. So whenever agent-os was importable, source 3 of stage 3 contributed nothing: no response ever received a content-derived sensitivity tag, and classification silently degraded to catalog annotations alone. With agent-os absent the fallback worked, which is why the existing tests, all written on the no-AGT path, stayed green. Changes: - Call `find_matches` and `find_pii_matches`, resolved with `getattr` so a future upstream rename degrades to a logged warning rather than a silent no-op. - Run the local patterns as a second pass rather than an either/or. Besides covering an AGT-side failure, this is the defence against microsoft/agent-governance-toolkit#3494, where a secret with a suffix glued to it (`AKIA..._old`) is not redacted at all. That issue's fix PR was closed unmerged on 2026-08-05, so it is unfixed upstream with no release pending. - Replace the blanket `except Exception: pass` with a narrowed handler that logs. Swallowing everything is what hid this. Three regression tests, each failing on the previous implementation and passing here: AGT missing the method, AGT raising, and the #3494 suffix case. Note: cmcp is not exposed to microsoft/agent-governance-toolkit#3496. That defect is in `MCPResponseScanner.sanitize_response`, reached only under `ResponsePolicy.SANITIZE`. `proxy.py` leaves the default `BLOCK`, and the pipeline calls `scan_response`, not `sanitize_response`. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Imran Siddique --- src/cmcp_runtime/inspection/pipeline.py | 57 ++++++++++++++++------ tests/unit/test_stage3_sensitivity.py | 65 +++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 16 deletions(-) diff --git a/src/cmcp_runtime/inspection/pipeline.py b/src/cmcp_runtime/inspection/pipeline.py index afe8d9d..4d5c129 100644 --- a/src/cmcp_runtime/inspection/pipeline.py +++ b/src/cmcp_runtime/inspection/pipeline.py @@ -288,7 +288,7 @@ def _classify_sensitivity( 1. catalog_entry.sensitivity_level: always applied 2. field-level x-sensitivity annotations in output_schema properties - 3. pattern matching on response content (AGT CredentialRedactor or regex fallback) + 3. pattern matching on response content (AGT CredentialRedactor and local patterns) """ tags: list[str] = [] @@ -312,22 +312,47 @@ def _classify_sensitivity( except (json.JSONDecodeError, ValueError): pass - # Source 3: content pattern matching + # Source 3: content pattern matching. + # + # AGT and the local patterns both run, rather than AGT displacing them. Two + # reasons. First, AGT's CredentialRedactor does not redact a secret at all + # when a suffix is glued to it (microsoft/agent-governance-toolkit#3494: + # `AKIA..._old` passes through whole), so treating it as sufficient leaves a + # live key untagged. Second, an either/or meant any breakage on the AGT path + # silently disabled content classification entirely instead of degrading to + # the local patterns. if response_text: + _SENSITIVE = ("pii", "confidential", "hipaa_phi", "mnpi") + if _AGT_AVAILABLE and _agt_redactor is not None: - try: - matches = _agt_redactor.find_credentials(response_text) - if matches and not any(t in tags for t in ("pii", "confidential", "hipaa_phi", "mnpi")): - tags.append("pii") - except Exception: # nosec B110 - pass - else: - # Regex fallback for when AGT is unavailable - for pattern, tag in _PII_PATTERNS: - if pattern.search(response_text) and tag not in tags: - tags.append(tag) - if len(tags) >= 4: # cap scan at 4 distinct tags - break + # find_matches covers secrets, find_pii_matches covers PII spans. + # Both are classmethods on CredentialRedactor. A previous version + # called `find_credentials`, which has never existed on that class + # in any agt-core release, so this whole branch raised AttributeError + # into the handler below and contributed nothing. + for method in ("find_matches", "find_pii_matches"): + finder = getattr(_agt_redactor, method, None) + if finder is None: + # Narrow and loud: an AGT API that moved should surface as a + # degraded stage, not as silent unconditional passing. + _log.warning( + "AGT CredentialRedactor has no %s(); " + "falling back to local patterns for content classification", + method, + ) + continue + try: + if finder(response_text) and not any(t in tags for t in _SENSITIVE): + tags.append("pii") + except Exception: # nosec B110 - never let detection break the pipeline + _log.warning("AGT CredentialRedactor.%s() failed", method, exc_info=True) + + # Local patterns always run, as a second pass rather than a fallback. + for pattern, tag in _PII_PATTERNS: + if len(tags) >= 4: # cap scan at 4 distinct tags + break + if pattern.search(response_text) and tag not in tags: + tags.append(tag) return tags @@ -339,7 +364,7 @@ class SensitivityClassificationStage: Applies three classification sources in order: 1. catalog_entry.sensitivity_level annotation 2. x-sensitivity field-level tags in output_schema properties - 3. Content pattern matching (AGT CredentialRedactor or regex fallback) + 3. Content pattern matching (AGT CredentialRedactor and local patterns) """ def run( diff --git a/tests/unit/test_stage3_sensitivity.py b/tests/unit/test_stage3_sensitivity.py index c383104..620ddeb 100644 --- a/tests/unit/test_stage3_sensitivity.py +++ b/tests/unit/test_stage3_sensitivity.py @@ -160,3 +160,68 @@ def test_no_duplicate_pii_from_catalog_and_field(): with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", False): result = stage.run({"ssn": "123-45-6789"}, _make_entry("pii", output_schema=schema)) assert result.sensitivity_tags.count("pii") == 1 + + +# ── Source 3 with AGT present: local patterns must still run (#476) ─────────── +# +# The previous implementation called `_agt_redactor.find_credentials()`, a method +# that has never existed on agt-core's CredentialRedactor (verified against the +# pinned 4.1.0 wheel: the real names are find_matches / find_pii_matches). The +# call raised AttributeError into a bare `except Exception: pass`, and because +# the local patterns sat in the `else` branch of that check they never ran +# either. With AGT installed, content-based classification was dead. + +class _RedactorMissingMethod: + """An agt-core CredentialRedactor without the method the caller expects.""" + + +class _RedactorRaising: + @staticmethod + def find_matches(_value: str) -> list[str]: + raise RuntimeError("upstream blew up") + + @staticmethod + def find_pii_matches(_value: str) -> list[str]: + raise RuntimeError("upstream blew up") + + +class _RedactorSuffixBlind: + """Reproduces microsoft/agent-governance-toolkit#3494: a secret with a + suffix glued to it is not matched at all, so AGT alone calls it clean.""" + + @staticmethod + def find_matches(value: str) -> list[str]: + return ["AKIA"] if "AKIAIOSFODNN7EXAMPLE" in value and "_old" not in value else [] + + @staticmethod + def find_pii_matches(_value: str) -> list[str]: + return [] + + +def test_local_patterns_still_run_when_agt_lacks_the_method(): + stage = SensitivityClassificationStage() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + result = stage.run( + {"data": "SSN is 123-45-6789"}, _make_entry(), _RedactorMissingMethod() + ) + assert "pii" in result.sensitivity_tags + + +def test_local_patterns_still_run_when_agt_raises(): + stage = SensitivityClassificationStage() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + result = stage.run( + {"data": "SSN is 123-45-6789"}, _make_entry(), _RedactorRaising() + ) + assert "pii" in result.sensitivity_tags + + +def test_suffixed_secret_still_tagged_despite_agt_3494(): + stage = SensitivityClassificationStage() + blind = _RedactorSuffixBlind() + with patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True): + clean = stage.run({"k": "AKIAIOSFODNN7EXAMPLE x@y.com"}, _make_entry(), blind) + suffixed = stage.run({"k": "AKIAIOSFODNN7EXAMPLE_old x@y.com"}, _make_entry(), blind) + # AGT catches the bare key; only the local pass catches the suffixed one. + assert "pii" in clean.sensitivity_tags + assert "pii" in suffixed.sensitivity_tags From 517d915f5355c11311a0dd8e9395ae7a5e1b339f Mon Sep 17 00:00:00 2001 From: Imran Siddique Date: Thu, 6 Aug 2026 10:34:26 -0700 Subject: [PATCH 2/2] fix(inspection): stop AGT integration failures from being silent Follow-on from the dead `find_credentials` call in the previous commit. Audited every AGT symbol and method cmcp calls against the pinned agt-core 4.1.0 wheel. `find_credentials` was the only phantom API; the rest resolve, including the `GovernancePolicy` re-export in `proxy.py`, which is a real module-level import from `agent_os.integrations.base` and works at runtime despite the type: ignore. What the audit did find is the handling around those calls. 1. All three AGT components were constructed in one try block, so a failure building the first left the other two unbuilt and all three silently None. One upstream API change would disable three security components at once with no log line. Each is now constructed independently and logs on failure. 2. `PromptInjectionDetector.detect()` failing fell through to the local patterns, which is correct, but silently. A broken detector was indistinguishable from a working one while the weaker starter set was what actually ran. Now logged with the pattern-set version that took over. 3. `MCPResponseScanner.scan_response()` failing was swallowed entirely. It correctly does not deny, since an errored scanner has produced no verdict and stage 4's own detection still runs, but losing MCP-specific threat coverage for a response should not be invisible. Now logged with the tool name. `catalog/scanner.py` already did this correctly and is unchanged: it logs, sets an explicit `_available = False`, and distinguishes not-installed from failed. Two tests, both failing on the previous commit: one pinning that a failing component no longer takes the others down, one asserting the warning is emitted. Not changed, flagged for review instead: when AGT's detector returns a clean verdict, stage 4 returns allow immediately and the local pattern set never runs. Making both run would be more conservative, but it changes deny behaviour and risks false positives in a gateway, so it wants a deliberate decision rather than being folded into a robustness fix. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Imran Siddique --- src/cmcp_runtime/inspection/pipeline.py | 51 ++++++++++++++++++++++--- tests/unit/test_stage3_sensitivity.py | 46 ++++++++++++++++++++++ 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/src/cmcp_runtime/inspection/pipeline.py b/src/cmcp_runtime/inspection/pipeline.py index 4d5c129..c95761d 100644 --- a/src/cmcp_runtime/inspection/pipeline.py +++ b/src/cmcp_runtime/inspection/pipeline.py @@ -222,8 +222,16 @@ def _stage4_injection_detection( injection_score=score, ) return StageResult(stage="injection", decision="allow") - except Exception: # nosec B110 - pass # Fall through to regex + except Exception: + # Falling through to the local patterns is the right behaviour, but + # doing it silently means a broken AGT detector looks identical to a + # working one while the weaker starter set is what actually ran. + _log.warning( + "AGT PromptInjectionDetector.detect() failed; " + "falling back to local patterns v%s", + _PATTERNS_VERSION, + exc_info=True, + ) # Fallback: regex patterns patterns = custom_patterns or _COMPILED_PATTERNS @@ -423,13 +431,35 @@ def __init__( self._agt_redactor: Any = None self._agt_response_scanner: Any = None if _AGT_AVAILABLE: + # Constructed independently. Sharing one try block meant a failure in + # the first component skipped the other two, so a single upstream API + # change disabled three security components at once, silently. try: _cfg = DetectionConfig(sensitivity=injection_sensitivity) self._agt_injection_detector = PromptInjectionDetector(config=_cfg) + except Exception: + _log.warning( + "AGT PromptInjectionDetector unavailable; " + "stage 4 falls back to local patterns v%s", + _PATTERNS_VERSION, + exc_info=True, + ) + try: self._agt_redactor = CredentialRedactor() + except Exception: + _log.warning( + "AGT CredentialRedactor unavailable; " + "stage 3 falls back to local patterns", + exc_info=True, + ) + try: self._agt_response_scanner = AGTResponseScanner() - except Exception: # nosec B110 - pass + except Exception: + _log.warning( + "AGT MCPResponseScanner unavailable; " + "stage 4 loses MCP-specific threat detection", + exc_info=True, + ) def run( self, @@ -543,8 +573,17 @@ def run( injection_scanner = "timeout" stage_results["injection"] = "deny" agt_mcp_denied = True - except Exception: # nosec B110 - pass + except Exception: + # Unlike the timeout above this does not deny, because a scanner + # that errored has produced no verdict either way and stage 4's + # own detection still runs below. It must not be silent though: + # MCP-specific threat coverage is gone for this response. + _log.warning( + "AGT MCPResponseScanner.scan_response() failed for %s; " + "no MCP-specific threat coverage on this response", + catalog_entry.tool_name, + exc_info=True, + ) # INJECT-002: wrap AGT PromptInjectionDetector with the same timeout bound. def _run_s4() -> StageResult: diff --git a/tests/unit/test_stage3_sensitivity.py b/tests/unit/test_stage3_sensitivity.py index 620ddeb..cb7bcff 100644 --- a/tests/unit/test_stage3_sensitivity.py +++ b/tests/unit/test_stage3_sensitivity.py @@ -225,3 +225,49 @@ def test_suffixed_secret_still_tagged_despite_agt_3494(): # AGT catches the bare key; only the local pass catches the suffixed one. assert "pii" in clean.sensitivity_tags assert "pii" in suffixed.sensitivity_tags + + +# ── AGT component construction is independent (#476) ────────────────────────── +# +# All three components were built in one try block, so a failure constructing +# the first left the other two unbuilt and all three silently None. One upstream +# API change disabled three security components at once, with no log line. + +def test_one_failing_agt_component_does_not_disable_the_others(): + from cmcp_runtime.inspection.pipeline import InspectionPipeline + + def _boom(*_a: object, **_k: object) -> None: + raise RuntimeError("upstream API changed") + + with ( + patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True), + patch("cmcp_runtime.inspection.pipeline.DetectionConfig", _boom), + patch("cmcp_runtime.inspection.pipeline.CredentialRedactor", lambda: "redactor"), + patch("cmcp_runtime.inspection.pipeline.AGTResponseScanner", lambda: "scanner"), + ): + pipeline = InspectionPipeline() + + # The detector failed, but the other two must still be constructed. + assert pipeline._agt_injection_detector is None + assert pipeline._agt_redactor == "redactor" + assert pipeline._agt_response_scanner == "scanner" + + +def test_failing_agt_component_is_logged_not_swallowed(caplog: Any) -> None: + import logging + + from cmcp_runtime.inspection.pipeline import InspectionPipeline + + def _boom(*_a: object, **_k: object) -> None: + raise RuntimeError("upstream API changed") + + with ( + caplog.at_level(logging.WARNING, logger="cmcp_runtime.inspection.pipeline"), + patch("cmcp_runtime.inspection.pipeline._AGT_AVAILABLE", True), + patch("cmcp_runtime.inspection.pipeline.DetectionConfig", _boom), + patch("cmcp_runtime.inspection.pipeline.CredentialRedactor", lambda: "redactor"), + patch("cmcp_runtime.inspection.pipeline.AGTResponseScanner", lambda: "scanner"), + ): + InspectionPipeline() + + assert any("PromptInjectionDetector unavailable" in r.message for r in caplog.records)