Skip to content

fix(inspection): stage 3 content classification was dead when AGT is installed - #477

Open
imran-siddique wants to merge 2 commits into
mainfrom
fix/stage3-dead-credential-detection
Open

fix(inspection): stage 3 content classification was dead when AGT is installed#477
imran-siddique wants to merge 2 commits into
mainfrom
fix/stage3-dead-credential-detection

Conversation

@imran-siddique

Copy link
Copy Markdown
Contributor

What this fixes

_classify_sensitivity called _agt_redactor.find_credentials(response_text). That method has never existed on agt-core's CredentialRedactor. I checked the pinned 4.1.0 wheel and every commit in AGT history; the real API is find_matches (secrets) and find_pii_matches (PII spans).

# before
if _AGT_AVAILABLE and _agt_redactor is not None:
    try:
        matches = _agt_redactor.find_credentials(response_text)   # AttributeError, always
        ...
    except Exception:  # nosec B110
        pass                                                      # swallowed
else:
    for pattern, tag in _PII_PATTERNS:                            # never reached with AGT present
        ...

The AttributeError went into a bare except Exception: pass, and the local pattern sweep sat in the else branch, so it did not run either. Whenever agent-os was importable, source 3 of stage 3 contributed nothing. No response received a content-derived sensitivity tag; classification silently fell back to catalog annotations alone.

The existing tests all patch _AGT_AVAILABLE to False, so they exercised the fallback and stayed green throughout.

Changes

  • Call find_matches and find_pii_matches, resolved via getattr so a future upstream rename degrades to a logged warning instead of a silent no-op.
  • Run the local _PII_PATTERNS as a second pass rather than an either/or.
  • Replace the blanket except Exception: pass with a narrowed, logging handler. Swallowing everything is what hid this.

Why the second pass, and not just the corrected method name

microsoft/agent-governance-toolkit#3494: AGT's redactor does not redact a secret at all when a suffix is glued to it, so AKIAIOSFODNN7EXAMPLE_old passes through whole. That issue's fix PR was closed unmerged on 2026-08-05 and there is no release pending, so relying on AGT alone leaves a live key untagged.

Tests

Three regression tests, each verified failing on the previous implementation and passing here:

Test Scenario
test_local_patterns_still_run_when_agt_lacks_the_method AGT object without the expected method
test_local_patterns_still_run_when_agt_raises AGT method raising
test_suffixed_secret_still_tagged_despite_agt_3494 the _old suffix case

tests/unit/test_stage3_sensitivity.py and tests/unit/test_inspection.py: 56 passed. ruff clean, mypy clean.

Not affected

cmcp is not exposed to #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.

Note on the local test environment

pytest tests/unit shows 21 collection errors on this branch and on pristine main alike. They come from a dev checkout of AGT shadowing the pinned agt-core 4.1.0, so agent_os.policies.backends and mcp_gateway.GovernancePolicy are absent. That is #472 reproduced locally, and worth noting there: on 5.x, proxy.py loses GovernancePolicy as well as the evaluator losing CedarBackend.

Refs #471, #472.

imran-siddique and others added 2 commits August 6, 2026 09:09
…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) <noreply@anthropic.com>
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
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) <noreply@anthropic.com>
Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
@imran-siddique

Copy link
Copy Markdown
Contributor Author

Second commit: the handlers that hid the first bug

Audited every AGT symbol and method cmcp calls against the pinned agt-core 4.1.0 wheel, since a phantom method surviving in main suggested the pattern might repeat.

Good news on the API surface. find_credentials was the only one that does not exist. Everything else resolves against 4.1.0:

Call site Symbol / method 4.1.0
catalog/scanner.py MCPSecurityScanner.{register_tool, scan_tool, check_rug_pull} ok
inspection/pipeline.py PromptInjectionDetector.detect, DetectionConfig ok
inspection/pipeline.py MCPResponseScanner.scan_response ok
mcp/proxy.py MCPGateway.{intercept_tool_call, intercept_tool_response} ok
mcp/proxy.py GovernancePolicy ok, re-export
mcp/server.py StatelessKernel ok
policy/evaluator.py CedarBackend ok on 4.x

On GovernancePolicy: it is defined in agent_os.integrations.base, and mcp_gateway.py imports it at module level, so from agent_os.mcp_gateway import GovernancePolicy works at runtime. The # type: ignore[attr-defined] is legitimate, not papering over a break.

What the audit did find is the error handling around those calls, all in pipeline.py:

  1. One try block built all three components. A failure constructing the first left the other two unbuilt and all three silently None. A single upstream API change would disable three security components at once with no log line. Now constructed independently, each logging on failure.
  2. detect() failing fell through to local patterns silently. The fallthrough is correct; the silence is not. A broken detector looked identical to a working one while the weaker starter set was what actually ran. Now logged with the pattern-set version that took over.
  3. scan_response() failing was swallowed entirely. It correctly does not deny, since an errored scanner produced no verdict and stage 4's own detection still runs, but losing MCP-specific coverage for a response should not be invisible. Now logged with the tool name.

catalog/scanner.py already did this properly and is unchanged. It logs, sets an explicit _available = False, and distinguishes not-installed from failed. That is the pattern the other two now follow.

Two more tests, both verified failing on the previous commit. 58 pass across test_stage3_sensitivity.py and test_inspection.py; ruff and mypy clean.

One thing deliberately not changed

When AGT's detector returns a clean verdict, stage 4 returns allow immediately and the local pattern set never runs. Only the exception path falls back, despite the docstring reading as though the regex set is a general fallback.

Running both and denying if either denies would be more conservative and consistent with the stage 3 fix in the first commit. But it changes deny behaviour and risks false positives in a gateway, and patterns_version is reported in every InspectionResult as the set that made the decision, which is already misleading when AGT short-circuits. That is a design call, so I have left it for review rather than folding it into a robustness fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant