Skip to content

fix(adapters): address review feedback from #1593 — error suppression, session IDs, test bug#1607

Closed
miyannishar wants to merge 1 commit into
microsoft:mainfrom
miyannishar:fix/pr-1593-review-feedback
Closed

fix(adapters): address review feedback from #1593 — error suppression, session IDs, test bug#1607
miyannishar wants to merge 1 commit into
microsoft:mainfrom
miyannishar:fix/pr-1593-review-feedback

Conversation

@miyannishar
Copy link
Copy Markdown
Collaborator

Summary

Addresses all 5 issues raised by @imran-siddique in #1593 before it was closed and rebased as #1605. The original native-hook adapter code has already been merged via #1605; this PR contains only the correctness fixes from the review.


Fixes Applied

🔴 Blocking — Error suppression

Files: semantic_kernel_adapter.py (wrap_kernel()), pydantic_ai_adapter.py (module-level wrap())

contextlib.suppress(Exception) was silently swallowing all exceptions, not just DeprecationWarning. This meant a real failure (invalid kernel, failed policy validation) inside wrap() would return None with no indication of the problem.

Fix: Removed contextlib.suppress(Exception). Both functions now use only warnings.catch_warnings() to suppress the nested DeprecationWarning.

# Before (broken — swallows ALL exceptions)
with contextlib.suppress(Exception), warnings.catch_warnings():
    warnings.simplefilter("ignore", DeprecationWarning)
    return wrapper.wrap(kernel)

# After
with warnings.catch_warnings():
    warnings.simplefilter("ignore", DeprecationWarning)
    return wrapper.wrap(kernel)

🔴 Bug — Test test_blocks_pattern_in_observation

File: tests/test_smolagents_hooks.py

The test incorrectly assumed callback(step_with_blocked_obs, agent) would pass when the step had no tool-call action. GovernanceStepCallback scans observations unconditionally, so it always raises for a blocked observation regardless of whether tool calls are present.

Fix: Rewrote the test to correctly assert that PolicyViolationError is raised in both the no-tool-call and the allowed-tool-call-with-blocked-observation cases.


🟡 Warning — Double deprecation in wrap_client()

File: anthropic_adapter.py

wrap_client() called kernel.wrap() without suppressing the inner deprecation, so users saw two DeprecationWarning messages per invocation.

Fix: Wrapped the inner call with warnings.catch_warnings() to suppress the nested warning, matching the pattern already used in SK/PydanticAI.


🟡 Warning — Session ID collisions

Files: anthropic_adapter.py, semantic_kernel_adapter.py

Session IDs used int(time.time()), causing two hooks created within the same second to share a session ID and corrupt audit trails.

Fix: Replaced with uuid.uuid4().hex[:12] for guaranteed uniqueness.

# Before
session_id=f"ant-{int(time.time())}"

# After
session_id=f"ant-{uuid.uuid4().hex[:12]}"

🟡 Warning — as_filter() overwrites context on repeated calls

File: semantic_kernel_adapter.pyGovernanceFunctionFilter.__init__

The context was stored under the hardcoded key "sk-filter", so calling as_filter() twice on the same wrapper silently discarded the first filter's execution context.

Fix: The context key is now the same uuid-based _filter_id generated per-instance, so each filter has its own isolated entry in wrapper._contexts.


Testing

  • All four changed files parse cleanly (ast.parse)
  • grep confirms zero remaining contextlib.suppress(Exception) or int(time.time()) session IDs across the three adapter files
  • test_smolagents_hooks.py now correctly specifies the unconditional observation-scan behaviour

Closes the feedback loop on #1593.

Fixes 5 issues raised by @imran-siddique:

BLOCKING:
- SK wrap_kernel(), PydanticAI wrap(): remove contextlib.suppress(Exception)
  which was silently swallowing ALL exceptions (not just DeprecationWarning).
  Now uses warnings.catch_warnings() alone to suppress only the nested
  DeprecationWarning, so real errors (invalid kernel, policy validation
  failures) propagate correctly.

BUG:
- test_smolagents_hooks: test_blocks_pattern_in_observation incorrectly
  assumed a step with no tool calls but a blocked observation would pass.
  GovernanceStepCallback scans observations unconditionally; the test now
  correctly asserts PolicyViolationError is raised in both cases (no-tool-
  call step and allowed-tool-call step with blocked observation).

WARNINGS:
- Anthropic wrap_client(): suppress nested DeprecationWarning from kernel.wrap()
  so callers see only one warning instead of two.
- Anthropic wrap()/as_message_hook(), SK GovernanceFunctionFilter: replace
  int(time.time()) session IDs with uuid.uuid4().hex[:12] to prevent
  same-second session ID collisions in audit trails.
- SK GovernanceFunctionFilter: use the uuid-based ID as the _contexts dict
  key so calling as_filter() multiple times no longer overwrites the first
  filter's context entry.

Part of: microsoft#1593
@github-actions
Copy link
Copy Markdown

🤖 AI Agent: security-scanner — View details

No security issues found.

@github-actions github-actions Bot added the tests label Apr 30, 2026
@github-actions
Copy link
Copy Markdown

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

Docs Sync

  • wrap_kernel() in semantic_kernel_adapter.py -- missing docstring
  • wrap() in pydantic_ai_adapter.py -- missing docstring
  • wrap_client() in anthropic_adapter.py -- missing docstring
  • GovernanceFunctionFilter.__init__() in semantic_kernel_adapter.py -- missing docstring
  • README.md -- update required to reflect changes in error handling, session ID generation, and GovernanceFunctionFilter behavior
  • CHANGELOG.md -- missing entry for fixes and behavioral changes

@github-actions github-actions Bot added the size/M Medium PR (< 200 lines) label Apr 30, 2026
@github-actions
Copy link
Copy Markdown

🤖 AI Agent: code-reviewer — Action Items:

TL;DR: 0 blockers, 3 warnings. Solid fixes; minor improvements can follow later.

# Sev Issue Where
1 🟡 Double DeprecationWarning suppression needed anthropic_adapter.py
2 🟡 Session ID collisions due to int(time.time()) anthropic_adapter.py, semantic_kernel_adapter.py
3 🟡 Context key overwrites on repeated as_filter() calls semantic_kernel_adapter.py

Action Items:

  • None. No blockers identified.

Warnings (fine as follow-up PRs):

# Issue Suggested Follow-Up
1 Double DeprecationWarning suppression needed Ensure consistent deprecation handling across all adapters.
2 Session ID collisions due to int(time.time()) Audit for other potential non-unique ID generation patterns.
3 Context key overwrites on repeated as_filter() calls Review other context management for similar hardcoded keys.

@github-actions
Copy link
Copy Markdown

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

API Compatibility

Severity Change Impact
🔴 High Removed contextlib.suppress(Exception) in wrap_kernel() and wrap() functions. Code relying on silent suppression of exceptions other than DeprecationWarning may now raise unexpected errors.
🟡 Medium Changed session ID generation from int(time.time()) to uuid.uuid4().hex[:12]. Code relying on predictable or timestamp-based session IDs may break.
🟡 Medium Updated GovernanceFunctionFilter to use unique _filter_id for context keys instead of a hardcoded "sk-filter". Code that directly accesses or modifies the context using the old hardcoded key will break.

@github-actions
Copy link
Copy Markdown

🤖 AI Agent: test-generator — `semantic_kernel_adapter.py`

semantic_kernel_adapter.py

  • test_wrap_kernel_error_handling -- Validate that wrap_kernel() raises an exception for invalid kernels instead of suppressing it.
  • test_governance_function_filter_context_isolation -- Ensure multiple GovernanceFunctionFilter instances do not overwrite each other's contexts.

pydantic_ai_adapter.py

  • test_wrap_error_handling -- Confirm that wrap() raises an exception for invalid agents instead of suppressing it.

anthropic_adapter.py

  • test_wrap_client_single_deprecation -- Verify that wrap_client() emits only one DeprecationWarning per invocation.
  • test_unique_session_ids -- Ensure session IDs generated by uuid.uuid4() are unique across multiple calls.

test_smolagents_hooks.py

  • test_blocks_pattern_in_observation -- Verify that PolicyViolationError is raised for both tool-call and no-tool-call steps with blocked observations.

@github-actions
Copy link
Copy Markdown

PR Review Summary

Check Status Details
🔍 Code Review ❌ Failed Issues detected
🛡️ Security Scan ✅ Completed Analysis complete
🔄 Breaking Changes ❌ Failed Issues detected
📝 Docs Sync ❌ Failed Issues detected
🧪 Test Coverage ❌ Failed Issues detected

Verdict: ❌ Changes needed

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

Labels

size/M Medium PR (< 200 lines) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant