Repository navigation
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. The fix improves security by addressing a prompt-injection bypass and ensures fail-closed behavior for non-converging sanitization.
Action items: None, as no blockers were identified. Warnings: |
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: test-generator — `src/agent_os/mcp_response_scanner.py`
|
🤖 AI Agent: contributor-guide — Actionable Items:
Welcome, and thank you for your contribution! 🎉 Your detailed explanation and thorough testing are impressive. The changes improve security and performance effectively. Actionable Items:
Refer to CONTRIBUTING.md for guidance. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
There was a problem hiding this comment.
Pull request overview
TL;DR: 0 blockers, 1 warning.
| # | Sev | Issue | Where |
|---|---|---|---|
| 1 | Warn | Avoid double-scanning the response per strip pass by combining “record match” + “strip” into a single re.sub callback |
agent_os/mcp_response_scanner.py::sanitize_response |
Fixes an MCP prompt-injection bypass by making MCPResponseScanner.sanitize_response strip instruction tags to a bounded fixed point (fail-closed on non-convergence) and by extending MCPGateway’s SANITIZE-mode residual re-check to include instruction_injection tags.
Changes:
- Make instruction-tag stripping iterative to a bounded fixed point (
_MAX_TAG_STRIP_PASSES) and fail closed if it doesn’t converge. - Add a SANITIZE-mode “residual instruction tag” re-scan in
MCPGatewaybefore allowing sanitized output through. - Add regression + performance-bounding tests covering splice/nesting constructions and non-converging inputs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
agent-governance-python/agent-os/src/agent_os/mcp_response_scanner.py |
Strip instruction tags to a bounded fixed point; fail closed if stripping doesn’t converge. |
agent-governance-python/agent-os/src/agent_os/mcp_gateway.py |
Re-scan sanitized output for residual instruction_injection tags before allowing. |
agent-governance-python/agent-os/tests/test_mcp_response_scanner.py |
Add regression + convergence/perf tests for tag splicing/nesting and fail-closed behavior. |
agent-governance-python/agent-os/tests/test_mcp_pii_and_response_gateway.py |
Add end-to-end gateway coverage for spliced tags and non-converging sanitization. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Minor:
- mcp_response_scanner.py fixed-point loop: a payload converging exactly on pass 8 is still blocked (loop cannot observe convergence on its final pass), so tolerated depth is _MAX-1. Fail-closed, fine; add a comment or use _MAX+1 range.
- tests/test_mcp_response_scanner.py test_sanitize_response_bounds_adversarial_nesting_cost: the wall-clock <1.0s assertion is a CI-flake risk (25x headroom measured, so low).
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Minor:
- mcp_response_scanner.py fixed-point loop: a payload converging exactly on pass 8 is still blocked (loop cannot observe convergence on its final pass), so tolerated depth is _MAX-1. Fail-closed, fine; add a comment or use _MAX+1 range.
- tests/test_mcp_response_scanner.py test_sanitize_response_bounds_adversarial_nesting_cost: the wall-clock <1.0s assertion is a CI-flake risk (25x headroom measured, so low).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
agent-governance-python/agent-os/src/agent_os/mcp_response_scanner.py:229
- The fail-closed log message reports
_MAX_TAG_STRIP_PASSESas the number of passes, but the loop actually executes_MAX_TAG_STRIP_PASSES + 1iterations (includes a confirming pass). This makes the log output misleading when diagnosing non-convergence.
logger.error(
"MCP response sanitization did not converge in %s passes for tool %s "
"— failing closed",
_MAX_TAG_STRIP_PASSES,
tool_name,
agent-governance-python/agent-os/src/agent_os/mcp_gateway.py:379
residual_tagusesscan_response(...), which reruns all scanners (imperative patterns, credential matching, PII, exfiltration URLs) even though this check only needs to detect residualinstruction_injectiontags. For large tool outputs this adds avoidable CPU cost on the SANITIZE hot path.
residual_tag = any(
threat.category == "instruction_injection"
for threat in self._response_scanner.scan_response(
sanitized, tool_name
).threats
)
|
Fixed in Reproduced by construction over Agreed that failing closed either way was safe — this is a correctness fix for what the constant means, not for the guarantee. Both the constant and the loop now say why the On the wall-clock assertion: measured it rather than loosening it. The unbounded fixed-point loop this replaced took ~15s on that input; the bounded version measures ~28ms over 5 runs, so 1.0s is ~35x headroom and sits far from both. The threshold is doing a coarse job — separating "bounded" from "quadratic" — so I documented that reasoning in the test instead of tightening it toward the measurement, which is what would actually make it flaky. Happy to swap it for a pass-count assertion if you would rather have no wall-clock in CI at all. 64 passed across the scanner and gateway modules. |
c3b08b4 to
61361e3
Compare
|
Rebased onto The conflict was in
No behaviour changed in the rebase — only the import line and the string quoting in the two new tests. Re-verified after rebasing:
Worth noting for anyone reproducing locally: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
agent-governance-python/agent-os/tests/test_mcp_response_scanner.py:196
- Avoid
is Truefor boolean assertions; prefer asserting the boolean directly to avoid identity-based comparisons.
assert [threat.category for threat in removed] == [
"instruction_injection"
] * depth
assert sanitized == "PAYLOAD"
assert scanner.scan_response(sanitized, "tool").is_safe is True
agent-governance-python/agent-os/src/agent_os/mcp_response_scanner.py:229
- The log message says sanitization "did not converge in %s passes" but it interpolates
_MAX_TAG_STRIP_PASSES, while the loop actually runs_MAX_TAG_STRIP_PASSES + 1iterations (extra convergence-check pass). This makes the reported count misleading/off-by-one. Consider wording this in terms of the depth limit instead of "passes".
"MCP response sanitization did not converge in %s passes for tool %s "
"— failing closed",
_MAX_TAG_STRIP_PASSES,
tool_name,
)
agent-governance-python/agent-os/tests/test_mcp_response_scanner.py:152
- Avoid
is Truefor boolean assertions; it can be brittle ifis_safeis ever a truthy bool-like value rather than the singletonTrue. Prefer asserting the boolean directly.
This issue also appears on line 191 of the same file.
assert removed
assert scanner.scan_response(sanitized, "tool").is_safe is True
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- BLOCKER (unchanged from the previous review; the push fixed both nits but not this): agent-governance-python/agent-os/src/agent_os/mcp_gateway.py residual re-scan still filters only category=="instruction_injection"; an error-returning scanner on the second call still yields allowed=True, action="sanitized" (reproduced at this head). One-line fix: threat.category in ("instruction_injection", "error").
Minor:
- both nits are verified fixed: _MAX+1 convergence loop with depth-8 test; wall-clock threshold justified.
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Minor:
- everything else at this head is verified sound: fixed-point convergence, depth boundary, non-echo of payload, error-category in sanitize_response itself; 64 tests pass.
|
Done — force-pushed The rebase is content-identical. I rebased onto the upstream merge base ( Same tree hash, empty diff — so the approve-grade content you reviewed at Re-ran the tests at the new head. Both files green, including the gateway file (I had to |
The two tests added in 39e3442 used `rescan` in their names, which the spell check rejects. The word is not a term worth a dictionary entry -- the surrounding docstrings and assertions already say "re-scan", so the identifiers now match the prose instead of coining a variant. Renames only: `test_failed_residual_re_scan_blocks_instead_of_allowing`, `fail_on_re_scan`, `test_residual_re_scan_that_raises_fails_closed`, `_ReScanRaises`. No assertion or behaviour changes; the file's 47 tests pass unchanged. Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
|
The To clear it, add |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
agent-governance-python/agent-os/src/agent_os/mcp_gateway.py:408
- When the residual re-scan finds (or fails closed with) a post-sanitization blocking category, the gateway blocks but still returns/audits only the pre-sanitization
threat_dicts. That means the decision/audit can omit the actual blocking reason (e.g. adata_exfiltrationURL created by tag splicing), which makes incident triage and policy tuning much harder.
Consider capturing the residual scan’s blocking threats (category + description only) and appending them to decision.threats when fail-closing, so the audit log reflects what actually triggered the block.
residual_tag = not (sanitize_failed or residual_credential)
if residual_tag:
try:
residual_tag = any(
threat.category in residual_block_categories
…hecks The Security Audit Required gate treats agent_os/mcp_gateway as a capability path, so this change needs a doc under docs/security/audits. Covers the three required sections: what changed and why (tag stripping splices the text around the removed tag, so one pass is not enough and the post-sanitize text is new text nothing had scanned), threat model impact (every outcome change moves allow to block; the bound on the fixed point also removes the CPU-exhaustion primitive an unbounded loop would have added), and test coverage (each regression test, and what keeps it from passing for the wrong reason). Records the 9 pre-existing agent-os failures and confirms they reproduce at the merge base with the same count. Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
github-actions
Bot
added
size/XL
|
liamcrumm thanks — added in Since the doc is what you'll review this change through, the three things it argues: One root cause, three symptoms. Removing a substring splices the text on either side of it together, and the splice is text nothing had scanned. That gives the tag that re-forms when its inner twin is stripped, and the Why the pass count is What is deliberately not blocked. Test-wise the audit notes what keeps each test from passing for the wrong reason, since several could: the end-to-end splice test asserts Two things recorded honestly rather than smoothed over. The suite is 2725 passed / 90 skipped / 9 failed, and all 9 reproduce at the merge base One thing I can't clear from my side: all workflow runs on this branch sit at |
de3e0bd to
2fb7a20
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
agent-governance-python/agent-os/tests/test_mcp_response_scanner.py:227
- Avoid boolean identity comparisons (
is True) in assertions; use a truthy assertion (assert ...) (or== True) to match the repo’s Python CodeQL guidance and avoid brittle identity-style checks.
assert scanner.scan_response(sanitized, "tool").is_safe is True
agent-governance-python/agent-os/tests/test_mcp_response_scanner.py:161
- Avoid boolean identity comparisons (
is True) in assertions; use a truthy assertion (assert ...) (or== True) to match the repo’s Python CodeQL guidance and avoid brittle identity-style checks.
This issue also appears on line 227 of the same file.
assert scanner.scan_response(sanitized, "tool").is_safe is True
agent-governance-python/agent-os/src/agent_os/mcp_gateway.py:420
residual_tagis used both as a control flag (whether to run the re-scan) and later as the result (whether residual threats were found). Splitting this into two variables avoids accidental logic mistakes in future edits and makes the intent clearer.
residual_tag = not (sanitize_failed or residual_credential)
if residual_tag:
try:
residual_tag = any(
threat.category in residual_block_categories
|
MohammadHaroonAbuomar DCO is fixed at All 8 commits already carried a One commit had a different author email than its own sign-off: the numeric-prefix form of my noreply address as author, the plain form in the trailer. DCO requires them to match, so it reports that commit as unsigned. Fixed with an Also at this head, in response to liamcrumm: Two things from your earlier reviews that the audit doc now states explicitly, since they were your findings and belong in the permanent record rather than only in a thread:
On the wall-clock flake risk you flagged: the threshold is 1.0s against a measured ~30ms (~35x headroom) versus ~15s for the unbounded loop it replaced. It is set to separate "bounded" from "quadratic" on a loaded runner rather than to be tight, and that reasoning is now in the test comment. The |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
agent-governance-python/agent-os/src/agent_os/mcp_response_scanner.py:56
- This comment block has an ambiguous reference: “Iterating exactly this many times” reads like it refers to
_MAX_TAG_NESTING_DEPTH + 1, but the off-by-one explanation is about iterating_MAX_TAG_NESTING_DEPTHtimes. Clarifying the constant name here will prevent misreading the bound logic.
# depth n needs n stripping passes plus one confirming pass; the loop below ranges
# over ``_MAX_TAG_NESTING_DEPTH + 1`` for that reason. Iterating exactly this many
# times instead would reject a payload at exactly this depth -- the final pass
# strips the last tag but the loop ends before it can see the result was clean --
# making the real limit one less than the constant says.
agent-governance-python/agent-os/src/agent_os/mcp_gateway.py:412
- The residual re-scan currently discards the residual scan’s findings (it only sets a boolean). If a hard-block threat is created by sanitization (e.g., the URL splice case), the decision/audit will block but
decision.threats(and the audit’s category list) may omit the category that actually caused the post-sanitize block, making incident triage harder.
threat.category in residual_block_categories
for threat in self._response_scanner.scan_response(
sanitized, tool_name
).threats
)
|
MohammadHaroonAbuomar the DCO objection is resolved -- status note so the All 10 commits at head Verified per-commit against each The content you called approve-grade is unchanged in substance. The residual re-scan still fails closed and still covers the hard-block categories: residual_block_categories = hard_block_categories | {
"instruction_injection",
"error",
}( |
|
The open threads are anchored to force-pushed commits that no longer exist on the branch, which is why they still render unresolved. At head Resolving the stale threads. Nothing further needed here. |
|
MohammadHaroonAbuomar Thanks — glad the content reads as approve-grade. On the DCO item, I think that was against an older head: all 10 commits on I did rebase Two things worth noting so this doesn't loop again:
Local suites at this head: 68 passed ( If your DCO tooling still reports 7 of 8 missing, could you paste which SHAs it names? That would tell me whether it's looking at a cached head or at the pre-rebase commits, and I'll fix whichever it is. Otherwise this should be ready to un-block. |
|
The DCO ask is already satisfied at this head — no rebase needed. All 10 commits on the branch carry a No output — zero unsigned commits out of 10. The Developer Certificate of I think the "7 of 8 commits" count was read from an earlier head. The two Since the trailers are already present I have not force-pushed. Rebasing an open On the two code asks from the earlier reviews, both are in at this head:
Happy to take another look if anything above doesn't match what you're seeing. |
|
Closing per maintainer decision; this repository is not accepting submissions from this account. |
Fixes #3496
Description
MCPResponseScanner.sanitize_responsestripped instruction tags in a single pass. Removing a tag splices the text on either side of it together, and that splice can form a new tag, so a response that writes an<important>tag with a second one embedded inside its own name survives sanitization — the inner tag is reported asstrippedwhile a live outer tag is handed back to the caller.Through
MCPGatewaywithResponsePolicy.SANITIZEthis was a prompt-injection bypass:allowed=True,action="sanitized", and content still carrying a working injection tag. The gateway's fail-closed re-check covered residual credentials but not residual tags, so nothing caught it.Changes
src/agent_os/mcp_response_scanner.py_MAX_TAG_STRIP_PASSES = 8.("", [MCPResponseThreat(category="error", ...)]), matching the module's existing fail-closed convention. The finding names the tool but never echoes the raw content.The bound is not cosmetic. An unbounded fixed-point loop is quadratic in the nesting depth —
"<" * n + "important>" * ncosts n passes over an O(n) string, measured at 14.9s for a 220 KB response. With the bound the same input is refused in 0.02s, and content a host would legitimately sanitize converges in one or two passes, so the bound costs nothing in the normal case.src/agent_os/mcp_gateway.pyinstruction_injection, mirroring the guarantee that already existed for credentials: re-scan the output rather than trusting thatsanitize_responseremoved everything it reported.sanitize_responsedeliberately never strips imperative prose, so blocking on fullis_safewould turnSANITIZEintoBLOCKfor ordinary text. This is stated in a comment at the call site.Tests
11 new tests, all of which fail on
main:test_sanitize_response_output_contains_no_instruction_tag— 6 payloads (splice at an interior offset, at the start of the tag name, the bracket form, two nesting levels, and a splice that forms a different tag than the one removed). Asserts the sanitizer's output satisfies the scanner it is paired with.test_sanitize_response_fails_closed_when_stripping_does_not_converge— beyond the bound the content is dropped entirely, the finding is anerror, and the payload is not echoed into it.test_sanitize_response_bounds_adversarial_nesting_cost— depth 20 000 completes in well under a second.test_sanitize_response_still_converges_in_one_pass_for_ordinary_content— the bound does not change the result for content that really needs sanitizing.test_spliced_injection_tag_does_not_reach_the_model— end to end throughMCPGateway.test_non_converging_response_is_blocked_not_sanitized— non-convergence surfaces asblocked, neversanitized.Verification
main, 9 of the new cases fail (54 passed) — confirming each one exercises the defect rather than passing incidentally.test_mcp_response_scanner.py,test_mcp_pii_and_response_gateway.py,test_mcp_gateway.py: 100 passed.tests/run: 257 failed / 4475 passed on this branch vs 257 failed / 4464 passed on the base — same failures, exactly the 11 new tests added. (The 257 pre-existing failures and 3 collection errors for missingagentmesh/agent_sreare present onmainand untouched here.)ruff checkandruff format --checkreport exactly the same diagnostics on these four files before and after the change; the diff introduces none.scripts/ci/changed_lines.py --mode added-linespiped tocspell@8.17.3) passes on the added lines.Checklist