Skip to content

fix(mcp-scan): use itertools.count for thread-safe request IDs#2

Merged
Jitha-afk merged 1 commit into
mainfrom
fix/thread-safe-request-id
May 22, 2026
Merged

fix(mcp-scan): use itertools.count for thread-safe request IDs#2
Jitha-afk merged 1 commit into
mainfrom
fix/thread-safe-request-id

Conversation

@AshikKuppili
Copy link
Copy Markdown
Collaborator

Description

Addresses blocker #1 and warnings 3-5 from code review on PR microsoft#2438:

All 133 MCP tests pass.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Maintenance (dependency updates, CI/CD, refactoring)
  • Security fix

Package(s) Affected

  • agent-os-kernel
  • agent-mesh
  • agent-runtime
  • agent-sre
  • agent-governance
  • docs / root

Checklist

  • My code follows the project style guidelines (ruff check)
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass (pytest)
  • I have updated documentation as needed
  • I have signed the Microsoft CLA

Attribution & Prior Art

  • This contribution does not contain code copied or derived from other projects without attribution
  • Any external projects that inspired this design are credited in code comments or documentation
  • If this PR implements functionality similar to an existing open-source project, I have listed it below

Prior art / related projects (if any):
N/A — uses standard library itertools.count and ssl module.

AI Assistance

  • I can explain every meaningful change in this PR: what it does, why, and what tradeoffs were considered
  • I have run tests and verification appropriate for this change
  • No part of this PR was autonomously submitted by an AI agent without my review
  • I have not used AI to generate review comments on others' PRs

If AI tools materially shaped this change, briefly note what was used:
GitHub Copilot CLI for implementation assistance — all output reviewed and tested locally (133/133 tests pass).

IP, Patents, and Licensing

  • This contribution does not implement patent-pending or patent-encumbered techniques
  • This contribution does not require an NDA or licensing agreement to understand or use
  • Any AI tools used have terms compatible with the MIT License

Related Issues

Addresses review feedback from microsoft#2438 (review)
Fixes blocker #1 and warnings 3-5 raised by @imran-siddique

@github-actions
Copy link
Copy Markdown

Welcome to the Agent Governance Toolkit! Thanks for your first pull request.
Please ensure tests pass, code follows style (ruff check), and you have signed the CLA.
See our Contributing Guide.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

🤖 AI Agent: code-reviewer — View details

TL;DR: 0 blockers, 0 warnings. Clean and secure fix.

# Sev Issue Where

No action items or warnings. Clean change.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

🤖 AI Agent: breaking-change-detector — API Compatibility

API Compatibility

Severity Change Impact
Medium MCP_PROTOCOL_VERSION now accepts multiple protocol versions, but issues a warning for older versions instead of rejecting them outright. Applications relying on strict rejection of older protocol versions may experience unexpected behavior.
Medium Added --no-verify-tls CLI flag, which disables TLS certificate verification. Potential security risk if users inadvertently disable TLS verification in production environments.
Low _next_request_id replaced with itertools.count for generating request IDs. If any external code relied on the previous _next_request_id implementation, it may break.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

🤖 AI Agent: test-generator — `agent-governance-python/agent-os/src/agent_os/cli/mcp_scan.py`

agent-governance-python/agent-os/src/agent_os/cli/mcp_scan.py

  • test_configure_tls_no_verify -- Test _configure_tls with verify=False to ensure _SSL_CONTEXT is set correctly.
  • test_configure_tls_verify -- Test _configure_tls with verify=True to ensure _SSL_CONTEXT is reset to None.
  • test_no_verify_tls_flag -- Validate that the --no-verify-tls flag correctly disables TLS verification.
  • test_protocol_warning -- Ensure a warning is raised for older but supported MCP protocol versions.

agent-governance-python/agent-os/tests/test_mcp_scan_cli.py

  • test_reset_handler_state -- Verify the autouse fixture resets class-level lists between tests.
  • test_protocol_rejection -- Add a test for rejecting unsupported MCP protocol versions.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

🤖 AI Agent: docs-sync-checker — Docs Sync

Docs Sync

  • def _configure_tls() in mcp_scan.py -- missing docstring
  • README.md -- section on CLI flags needs update to include --no-verify-tls
  • CHANGELOG.md -- missing entry for changes:
    • Replaced thread-unsafe _next_request_id with itertools.count
    • Added --no-verify-tls CLI flag
    • Updated MCP protocol version handling to support older versions with warnings

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

🤖 AI Agent: security-scanner — View details

No security issues found.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 21, 2026

PR Review Summary

Check Status Details
🔍 Code Review ✅ Passed No issues found
🛡️ Security Scan ✅ Passed No issues found
🔄 Breaking Changes ✅ Completed Analysis complete
📝 Docs Sync ✅ Completed Analysis complete
🧪 Test Coverage ✅ Completed Analysis complete

Verdict: ✅ Ready for human review

Replace mutable function-attribute counter with itertools.count(1).
Fixes thread-safety issue and prevents state leakage across test runs.

Signed-off-by: ashik.kuppili <kvashik5@gmail.com>
@AshikKuppili AshikKuppili force-pushed the fix/thread-safe-request-id branch from 702d4ae to ec2bf2b Compare May 22, 2026 06:21
@github-actions github-actions Bot added the tests label May 22, 2026
@Jitha-afk Jitha-afk merged commit dff49f5 into main May 22, 2026
117 of 121 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants