Skip to content

feat(orchestration): add content filtering and prompt shield module#174

Open
lenin-ribeiro wants to merge 30 commits into
mainfrom
feat/orchestration-filtering
Open

feat(orchestration): add content filtering and prompt shield module#174
lenin-ribeiro wants to merge 30 commits into
mainfrom
feat/orchestration-filtering

Conversation

@lenin-ribeiro

@lenin-ribeiro lenin-ribeiro commented Jun 19, 2026

Copy link
Copy Markdown

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Adds content filtering and prompt-shield support to the existing sap_cloud_sdk.aicore module. Azure Content Safety filtering and prompt-attack detection are activated automatically for every SAP AI Core model call made through LiteLLM.

Filtering is enabled by default when set_aicore_config() is called — no code change is required by the developer. The default policy blocks medium+ severity (Severity.MEDIUM) on every category and enables Prompt Shield on input for all sap/* model calls.

How it works

set_aicore_config() calls set_filtering() at the end, which patches litellm.GenAIHubOrchestrationConfig with a subclass (FilteringOrchestrationConfig) that:

  • Injects modules.filtering (Azure Content Safety config) into every v2 completion request body via transform_request
  • Detects filter rejections in responses and raises ContentFilteredError via transform_response
  • Unwraps filter rejections embedded in LiteLLM APIConnectionError exceptions via extract_filter_blocked()

LiteLLM still makes the HTTP call and Traceloop/OTel instrumentation is fully preserved.

Developer experience

Zero code change for the common case — existing agent code is unchanged:

from sap_cloud_sdk.aicore import set_aicore_config
set_aicore_config()
# ← filtering is now active at Severity.MEDIUM on all categories + prompt_shield=True

Thresholds configurable via env vars (set before set_aicore_config()):

AICORE_FILTER_SELF_HARM=0    # strict — block any detected self-harm content
AICORE_FILTER_ENABLED=false  # disable filtering entirely

Programmatic override at runtime uses the class-based API (mirrors the gen_ai_hub.orchestration.models.content_filtering shape):

from sap_cloud_sdk.aicore import (
    AzureContentFilter,
    ContentFiltering,
    InputFiltering,
    OutputFiltering,
    Severity,
    set_filtering,
)

set_filtering(ContentFiltering(
    input_filtering=InputFiltering(filters=[
        AzureContentFilter(
            hate=Severity.STRICT,
            violence=Severity.STRICT,
            sexual=Severity.STRICT,
            self_harm=Severity.STRICT,
            prompt_shield=True,
        ),
    ]),
    output_filtering=OutputFiltering(filters=[
        AzureContentFilter(
            hate=Severity.MEDIUM,
            violence=Severity.MEDIUM,
            sexual=Severity.MEDIUM,
            self_harm=Severity.MEDIUM,
        ),
    ]),
))

A direction can stack multiple filter providers — AzureContentFilter plus LlamaGuard38bFilter:

from sap_cloud_sdk.aicore import LlamaGuard38bFilter

set_filtering(ContentFiltering(
    input_filtering=InputFiltering(filters=[
        AzureContentFilter(prompt_shield=True),
        LlamaGuard38bFilter(hate=True, violent_crimes=True),
    ]),
))

Disable filtering at runtime:

from sap_cloud_sdk.aicore import disable_filtering

disable_filtering()

Handling blocked requests:

from sap_cloud_sdk.aicore import ContentFilteredError, extract_filter_blocked

try:
    result = await llm.ainvoke(messages)
except ContentFilteredError as e:
    return "Your request was blocked by content safety policy."
except Exception as e:
    if blocked := extract_filter_blocked(e):
        return "Your request was blocked by content safety policy."
    raise

Related Issue

N/A — new feature proposed and implemented by the App Foundation agent team.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

Run the unit tests:

uv run python -m pytest tests/aicore/ tests/core/unit/ -v
# Expected: 749 passed

Verify auto-activation:

import litellm
from sap_cloud_sdk.aicore import set_aicore_config
from sap_cloud_sdk.aicore.filtering._litellm_patch import FilteringOrchestrationConfig

set_aicore_config()
assert litellm.GenAIHubOrchestrationConfig is FilteringOrchestrationConfig
print("Filtering activated ✓")

Verify wire format:

from sap_cloud_sdk.aicore import (
    AzureContentFilter, ContentFiltering, InputFiltering, Severity,
)

cfg = ContentFiltering(
    input_filtering=InputFiltering(filters=[
        AzureContentFilter(self_harm=Severity.STRICT, prompt_shield=True),
    ]),
)
d = cfg.to_dict()
assert "input" in d
assert d["input"]["filters"][0]["type"] == "azure_content_safety"
assert d["input"]["filters"][0]["config"]["self_harm"] == 0
assert d["input"]["filters"][0]["config"]["prompt_shield"] is True
print("Wire format correct ✓")

Integration tests against a live AI Core deployment (skip when env absent):

# Requires AICORE_CLIENT_ID, AICORE_CLIENT_SECRET, AICORE_AUTH_URL, AICORE_BASE_URL,
# AICORE_RESOURCE_GROUP, AICORE_FILTER_TEST_MODEL, AICORE_FILTER_TEST_SELF_HARM_PROMPT
# set (or in .env_integration_tests).
uv run python -m pytest tests/aicore/integration/ -v

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes (68 filtering unit tests + 22 core/env tests + 4 integration scenarios)
  • All unit tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (aicore/user-guide.md)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Additional Notes

Public API surface (sap_cloud_sdk.aicore)

Symbol Purpose
set_aicore_config Load AI Core credentials and activate filtering (existing — now with side effect)
set_filtering Install a ContentFiltering configuration (or re-apply env defaults with no args)
disable_filtering Turn filtering off at runtime
ContentFiltering Top-level container with input_filtering + output_filtering
InputFiltering Direction container — accepts a list of filters
OutputFiltering Direction container — accepts a list of filters + optional stream_options
AzureContentFilter Azure Content Safety provider — 4 severity categories + prompt_shield
LlamaGuard38bFilter Llama Guard 3 8B provider — 14 boolean category flags
ContentFilter Abstract base — exposed for type hints / custom providers
Severity IntEnum of STRICT/LOW/MEDIUM/OFF (0/2/4/6)
ContentFilteredError Raised on filter rejection (direction, details, request_id)
OrchestrationError Base class for filter-related exceptions
extract_filter_blocked Unwrap input-filter rejections from LiteLLM APIConnectionError

All names available as from sap_cloud_sdk.aicore import … (flat).

New files

File Purpose
src/sap_cloud_sdk/core/env.py Typed env-var readers: read_env_str, read_env_bool, read_env_choice
src/sap_cloud_sdk/aicore/filtering/__init__.py Public filtering API: set_filtering, disable_filtering, re-exports
src/sap_cloud_sdk/aicore/filtering/_filters.py ContentFilter base, AzureContentFilter, LlamaGuard38bFilter
src/sap_cloud_sdk/aicore/filtering/_modules.py InputFiltering, OutputFiltering, ContentFiltering (with from_env())
src/sap_cloud_sdk/aicore/filtering/_models.py Severity enum
src/sap_cloud_sdk/aicore/filtering/_litellm_patch.py FilteringOrchestrationConfig subclass, _install(), extract_filter_blocked()
src/sap_cloud_sdk/aicore/filtering/exceptions.py ContentFilteredError, OrchestrationError
tests/core/unit/test_env.py 22 cases covering env-helper defaults, parsing, validation
tests/aicore/filtering/unit/test_filters.py 22 provider-class tests
tests/aicore/filtering/unit/test_modules.py 18 direction-container + from_env() tests
tests/aicore/filtering/unit/test_patch.py 15 LiteLLM patch + response-detection tests
tests/aicore/filtering/unit/test_filtering.py 10 set_filtering / disable_filtering behaviour tests
tests/aicore/filtering/unit/test_models.py 3 Severity enum tests
tests/aicore/integration/{conftest.py,filtering.feature,test_filtering_bdd.py} 4 live BDD scenarios — OFF baseline, ON benign, input filter STRICT, Prompt Shield jailbreak

Env vars reference

Variable Default Description
AICORE_FILTER_ENABLED true Set false to disable filtering entirely
AICORE_FILTER_DIRECTIONS input,output Which sides to filter
AICORE_FILTER_HATE 4 Azure severity 0/2/4/6
AICORE_FILTER_VIOLENCE 4 Azure severity 0/2/4/6
AICORE_FILTER_SEXUAL 4 Azure severity 0/2/4/6
AICORE_FILTER_SELF_HARM 4 Azure severity 0/2/4/6
AICORE_FILTER_PROMPT_SHIELD true Input-only jailbreak detection
AICORE_FILTER_TEST_MODEL n/a Model name used by the integration test suite (CI/operator)
AICORE_FILTER_TEST_SELF_HARM_PROMPT n/a Self-harm test prompt used by integration tests; kept out of source

Telemetry

Module.AICORE records four operations: set_aicore_config, set_filtering, disable_filtering, extract_filter_blocked. The previous Module.ORCHESTRATION was dropped — orchestration is folded into AI Core as a sub-package.

Restructure highlights (post-review)

This PR was substantially restructured in response to review feedback from @NicoleMGomes:

Env var prefix renamed from ORCH_FILTER_* to AICORE_FILTER_* for consistency with the new module location.

Class-based API (post-restructure)

The set_filtering() public signature was changed from keyword-driven (set_filtering(hate=..., violence=...)) to class-based (set_filtering(ContentFiltering(input_filtering=InputFiltering(filters=[AzureContentFilter(...)])))). This mirrors the shape used by generative-ai-hub-sdk (gen_ai_hub.orchestration.models.content_filtering.ContentFiltering + InputFiltering + OutputFiltering + AzureContentFilter + LlamaGuard38bFilter) so callers migrating from that SDK keep their call-site structure. Our threshold enum stays Severity.STRICT/LOW/MEDIUM/OFF rather than AzureThreshold.ALLOW_SAFE/ALLOW_SAFE_LOW/ALLOW_SAFE_LOW_MEDIUM/ALLOW_ALL.

AI-generated code disclosure

This contribution was developed with the assistance of Claude (Anthropic). All generated code has been reviewed and validated by the contributor. Tests were written alongside the implementation and verified to pass. Per the SAP AI contribution guideline.

Activates Azure Content Safety filtering and prompt attack detection
automatically for all SAP AI Core model calls. Filtering is enabled
by default when set_aicore_config() is called — no code change required
by the developer.

- New module sap_cloud_sdk.orchestration with:
  - FilteringModuleConfig: configures input/output filtering thresholds
    and prompt shield via ORCH_FILTER_* env vars (defaults: threshold 4,
    prompt_shield=True on input)
  - set_filtering(): programmatic override for thresholds at runtime
  - ContentFilteredError: raised when input or output is rejected by
    the content filter
  - extract_filter_blocked(): unwraps filter rejections embedded in
    LiteLLM APIConnectionError exceptions
- set_aicore_config() now calls _activate_filtering() at the end,
  applying FilteringModuleConfig.from_env() to LiteLLM's SAP provider
- Observability preserved: LiteLLM still makes the HTTP call;
  Traceloop/OTel instrumentation is unaffected
- 41 unit tests covering serialisation, env parsing, LiteLLM patch,
  response detection, and set_filtering() behaviour
- User guides updated in aicore/ and orchestration/; README breaking
  change notice added
- Version bump 0.27.1 → 0.28.0
@lenin-ribeiro lenin-ribeiro self-assigned this Jun 19, 2026
@lenin-ribeiro lenin-ribeiro requested a review from a team as a code owner June 19, 2026 14:57
Comment thread src/sap_cloud_sdk/orchestration/__init__.py Outdated
Comment thread src/sap_cloud_sdk/aicore/__init__.py
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
Comment thread src/sap_cloud_sdk/core/telemetry/operation.py Outdated
Comment thread src/sap_cloud_sdk/orchestration/__init__.py Outdated
Comment thread src/sap_cloud_sdk/aicore/filtering/_litellm_patch.py Outdated
Comment thread src/sap_cloud_sdk/orchestration/_models.py Outdated
Comment thread src/sap_cloud_sdk/orchestration/_models.py Outdated
read_env_str / read_env_bool / read_env_choice — used by the
filtering module for AICORE_FILTER_* runtime toggles. Distinct
from secret_resolver, which handles credential mount-and-fallback.
File moves only; imports are fixed in following commits.
Tests will fail at this commit and pass once Task 6 lands.
…env vars

- Replace Literal[0,2,4,6] with Severity IntEnum (STRICT/LOW/MEDIUM/OFF)
- Rename ORCH_FILTER_* -> AICORE_FILTER_* env vars
- Drop local _env/_env_bool/_env_severity helpers; use core.env
Update test imports and @patch() string targets to match the new
sap_cloud_sdk.aicore.filtering location. Rename ORCH_FILTER -> AICORE_FILTER
in test fixtures.
- Drop Module.ORCHESTRATION (14 modules -> 13)
- Drop ORCHESTRATION_SET_FILTERING operation
- Add AICORE_SET_FILTERING, AICORE_DISABLE_FILTERING,
  AICORE_EXTRACT_FILTER_BLOCKED under # AI Core Operations
- Update test_operation.py operation count (150 -> 152)
- Replace set_filtering(enabled=False) with dedicated disable_filtering()
- Add @record_metrics(AICORE, EXTRACT_FILTER_BLOCKED) on the parser
- Retarget set_filtering telemetry to Module.AICORE/AICORE_SET_FILTERING
- set_filtering args typed as Severity instead of Literal[0,2,4,6]
- Re-export set_filtering, disable_filtering, Severity, and exception
  types from sap_cloud_sdk.aicore
- Drop lazy import / try-except in set_aicore_config: no longer a
  circular-dep risk and any failure should surface, not be swallowed
- Update set_aicore_config docstring to describe filtering side effect
- Rename ORCH_FILTER_ENABLED -> AICORE_FILTER_ENABLED
- set_filtering(enabled=False) -> disable_filtering()
- Link points at aicore user guide content-filtering section
- Update env-var table to AICORE_FILTER_*
- Replace set_filtering(enabled=False) examples with disable_filtering()
- Show Severity enum usage in code examples
- Add Migration section showing the rename from sap_cloud_sdk.orchestration
- Update import paths to sap_cloud_sdk.aicore in all examples
- _litellm_patch: 'orchestration filtering' log msgs -> 'content filtering';
  remove unused # type: ignore[attr-defined] suppressions (ty no longer needs them)
- exceptions: module docstring + OrchestrationError docstring refer to the
  aicore.filtering location and clarify the class still serves as the base
  for orchestration-module errors (filtering today, grounding/masking later)
…t shield

Live tests against an AI Core orchestration endpoint with Azure Content
Safety. Skips cleanly when AICORE_* env vars are absent. Four scenarios:
OFF baseline, ON benign, input-filter STRICT block, Prompt Shield
jailbreak block.

The jailbreak prompt is taken verbatim from Microsoft Learn's Prompt
Shields documentation; the self-harm prompt is left as an empty
placeholder so operators can populate it from internal red-team fixtures
without committing harmful content to the public repository.
CI Code Quality job flagged these on commit 8a9babc:

- Ruff format: aicore/__init__.py, core/env.py, filtering/_models.py and
  several test files needed reformatting per the project's ruff config.
  Apply 'uv run ruff format' to all affected files. Drops unused imports
  (MagicMock, Mock, pytest) in tests/aicore/unit/test_aicore.py.

- ty check: 'cast: Callable[[str], T] = int  # type: ignore[assignment]'
  in core/env.py was rejected by CI's stricter ty. Drop the cast kwarg
  entirely (YAGNI — no caller passes anything other than int) and tighten
  TypeVar bound to int. Also resolves the integration test 'ctx.response
  has no attribute choices' error by typing ScenarioContext.response as Any.

- Wire AICORE_FILTER_TEST_SELF_HARM_PROMPT environment variable into the
  integration test's AZURE_TEST_PROMPTS dict so the operator can populate
  the self-harm scenario via GitHub secret without committing harmful
  content to the public repo. Scenario still skips when the var is unset.
… format

- core/env.py: drop the TypeVar from read_env_choice — CI ty rejects the
  T→int return-value coercion even with a type-ignore directive. The only
  caller (filtering/_models.py) wraps the result in Severity() anyway, so
  the generic typing was dead weight. Function now returns plain int.

- integration test 'Prompt Shield blocks a jailbreak attempt' was passing
  the request to Azure correctly (ContentFilteredError raised, direction
  input, request_id present) but my assertion looked for substring
  'prompt_shield' / 'jailbreak' in the details payload. Azure's actual
  wire format is:
      {'azure_content_safety': {'user_prompt_analysis': {'attack_detected': True}}}
  Rewrite the assertion (and the feature-file step) to check the
  structured 'attack_detected: True' flag — robust to wording shifts in
  Azure's response and a more meaningful semantic check.
Comment thread README.md Outdated
Comment thread src/sap_cloud_sdk/aicore/filtering/_litellm_patch.py Outdated
…uard38bFilter

Provider classes mirroring the gen_ai_hub.orchestration.models shape:
- ContentFilter abstract base with to_dict() that emits {type, config}
- AzureContentFilter: 4 severity categories + optional prompt_shield
- LlamaGuard38bFilter: 14 boolean category flags
Wire-format unchanged from FilteringModuleConfig path; this lays the
groundwork for replacing the kwarg-driven set_filtering API.
…iltering

Direction containers + top-level configuration mirroring gen-ai-hub-sdk:
- InputFiltering(filters=[...]) and OutputFiltering(filters=[...], stream_options=...)
- ContentFiltering(input_filtering=..., output_filtering=...) with to_dict()
- ContentFiltering.from_env() reads AICORE_FILTER_* env vars and builds
  a single AzureContentFilter per direction (replaces
  FilteringModuleConfig.from_env). prompt_shield applied only to input
  direction, matching server semantics.
Coupled changes that must land together (Tasks 3-6 of the plan):

- _models.py shrinks to just the Severity enum; ContentFilterConfig,
  PromptShieldConfig, FilteringModuleConfig are removed in favour of
  the class hierarchy added in earlier commits (_filters.py, _modules.py).
- _litellm_patch.py: _install annotation comment now reads
  'ContentFiltering | None'. Runtime type stays Any to avoid a circular
  import.
- aicore/filtering/__init__.py: set_filtering(config: ContentFiltering | None)
  takes a single positional arg. Old kwargs (hate, violence, sexual,
  self_harm, prompt_shield, directions) are removed. __all__ now exports
  the new 9-name class API; the previously-exposed ContentFilterConfig
  / PromptShieldConfig / FilteringModuleConfig are gone.
- aicore/__init__.py: re-exports updated; 13 public names total.
- test_filtering.py rewritten for the class API: TestSetFiltering covers
  no-args / explicit-None / explicit config / env-disable / explicit-config
  override / multi-filter cases. TestDisableFiltering unchanged.
- test_patch.py: TestInstall fixtures and TestTransformRequest cases
  build ContentFiltering objects instead of FilteringModuleConfig.
- test_models.py shrinks to TestSeverity (3 tests).

All 749 unit tests pass + 4 integration skipped. Wire format unchanged;
the live AI Core orchestration v2 endpoint sees identical JSON.
filtering_strict() and filtering_prompt_shield() now build
ContentFiltering(InputFiltering(filters=[AzureContentFilter(...)]))
and pass it to set_filtering. filtering_default() (no args) is
unchanged.
- Programmatic override now uses ContentFiltering / InputFiltering /
  OutputFiltering / AzureContentFilter
- New Multiple filter providers subsection introduces LlamaGuard38bFilter
- Migration subsection covers both the orchestration->aicore rename and
  the kwarg->class API shift
ty doesn't infer that 'assert cfg is not None' narrows cfg.input_filtering
from 'InputFiltering | None' to 'InputFiltering'. Add the explicit second
assertion in the two cases where we then access .filters[0] so the static
type-check passes alongside the dynamic check.
Address two review comments from @NicoleMGomes on the post-restructure
PR state:

- README.md: drop the breaking-change callout (review #9). The PR body
  + aicore user-guide carry the relevant migration detail; the README
  doesn't need release-notes-style content per change.
- _litellm_patch.py: tighten 'except Exception: pass' to 'except ValueError'
  around the json() call only (review #10). The previous broad except
  would swallow logic bugs in ContentFilteredError(...) construction;
  the narrower catch handles only the realistic failure mode
  (non-JSON response body) and lets logic errors surface. Applied
  symmetrically to both input-filter and output-filter detection
  branches.

Also satisfy CI's stricter ty by adding 'ty: ignore[too-many-positional-arguments]'
to two deliberately-failing positional-call sites in test_filters.py
(test_kwarg_only). The existing 'type: ignore[misc]' was sufficient for
mypy but not for the CI ty version.
Comment thread src/sap_cloud_sdk/aicore/filtering/__init__.py Outdated
Comment thread src/sap_cloud_sdk/aicore/filtering/_models.py Outdated
Comment thread README.md Outdated
- README: drop the 'Content Filtering & Prompt Shield' bullet from
  Key Features. It's a sub-feature of 'AI Core Integration' already
  on the list; other modules don't fan out their sub-features either.
- Move set_filtering / disable_filtering bodies from filtering/__init__.py
  to a new filtering/_api.py. filtering/__init__.py is now a thin
  re-export surface; logic lives in named modules.
- Inline Severity into _filters.py (where its only consumer
  AzureContentFilter lives) and delete _models.py — a one-enum file
  with a misleading 'models' name. TestSeverity moves to test_filters.py.

749 unit tests pass; flat 13-name public API at sap_cloud_sdk.aicore
is unchanged for external callers.
Comment thread src/sap_cloud_sdk/aicore/filtering/_api.py Outdated
Address review comment #14 ("can we have all public methods on a
filters.py? Better to read").

Merge _filters.py + _modules.py + _api.py into a single filters.py
(no underscore — signals the public surface). It now owns Severity,
ContentFilter, AzureContentFilter, LlamaGuard38bFilter, InputFiltering,
OutputFiltering, ContentFiltering, set_filtering, disable_filtering.
Tests import directly from .filters; the package __init__ stays a thin
re-export for the flat 'from sap_cloud_sdk.aicore import ...' path.

Internal-only files (_litellm_patch, exceptions) keep their separate
locations; they aren't part of the documented public surface.
CI's ty check started reporting 6 'unused ty: ignore directive' warnings
on b594bd6 that didn't appear on 8bcd53b — same ty==0.0.21 binary, but
the diagnostic count is non-zero and the pre-commit hook fails the build.

Drop the dead 'ty: ignore[...]' suppressions on pre-existing test sites
that ty no longer flags as errors:

- tests/adms/unit/test_client.py: 4 sites, MagicMock method-assign and
  union-attr patterns. The accompanying '# type: ignore[...]' for mypy
  is kept because mypy still needs it; only the ty annotation is removed.
- tests/adms/unit/test_http.py: 1 site, invalid-argument-type passing None
  to a strict-typed parameter inside pytest.raises.
- tests/adms/unit/test_query_options.py: 1 site, unknown-argument inside
  pytest.raises.
- tests/aicore/filtering/unit/test_filters.py: 2 sites, the
  too-many-positional-arguments directives I added earlier when CI's ty
  rejected deliberately-failing positional constructor calls — ty no
  longer flags these, so the suppressions are dead.

tests/destination/unit/test_client.py keeps its 2 'ty: ignore[invalid-argument-type]'
directives — ty still flags those as real errors there; the suppression
is doing genuine work to mask a deliberate test of validation rejection.
Code Quality CI runs 'uvx ty check .' which evaluates suppressions
differently from 'uv run ty check .' — same ty==0.0.21 binary but
different cache state. After commit 3e3edb7, CI reported 6 errors at
sites where I had removed 'ty: ignore[...]' directives but 'uv run ty'
locally said the directives were unused. After re-adding them, CI then
reported them as unused warnings — flapping.

Resolution: matched CI's invocation exactly ('uvx ty check .'), removed
suppressions where uvx ty flagged them as unused-ignore, kept (re-added
already in 3e3edb7) suppressions where uvx ty would flag the underlying
code as error. Verified locally with the same uvx command CI uses.

Sites cleared (suppressions no longer needed):
- tests/adms/unit/test_client.py (4 sites)
- tests/destination/unit/test_client.py (2 sites)
- tests/aicore/filtering/unit/test_filters.py (2 sites, my own additions)

Sites kept (suppressions still needed):
- tests/adms/unit/test_http.py:274 (invalid-argument-type)
- tests/adms/unit/test_query_options.py:86 (unknown-argument)

Also includes uv.lock SDK version bump 0.27.0 -> 0.28.0 (regenerated
from pyproject.toml; previously dirty in working tree).
class FilteringOrchestrationConfig(GenAIHubOrchestrationConfig):
"""GenAIHubOrchestrationConfig subclass that injects content filtering."""

def transform_request(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if transform_request is public, shouldn't it have the telemetry annotation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transform_request and transform_response are callbacks on LiteLLM's GenAIHubOrchestrationConfig contract — we override them, but they're invoked by LiteLLM, never by SDK users (the class isn't in __all__). The "public methods get telemetry" rule applies to our entry points (set_filtering, disable_filtering, extract_filter_blocked), all of which are decorated. Adding @record_metrics here would double-instrument
every LiteLLM request — every call already records its own LiteLLM span. Happy to add a counter on the ContentFilteredError raise sites if you'd like an explicit "filter blocked" metric, but a request-level decorator on the callback feels like the wrong place.

Comment thread src/sap_cloud_sdk/core/env.py Outdated
_TRUTHY = frozenset({"true", "1", "yes"})


def read_env_str(key: str, default: str = "") -> str:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this use secrets resolver?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be under core directly

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

secrets resolver is used for grabbing credentials, not env var from what I saw. we can go through with this change, but doesn't sound right to me

Comment thread src/sap_cloud_sdk/core/env.py Outdated
_TRUTHY = frozenset({"true", "1", "yes"})


def read_env_str(key: str, default: str = "") -> str:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be under core directly



@record_metrics(Module.AICORE, Operation.AICORE_SET_FILTERING)
def set_filtering(config: ContentFiltering | None = None) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge _litellm_patch with this filtering and have all the public interface in one place.

Also please check code guidelines under docs/. Usually we have the client concept

@NicoleGomes1 NicoleGomes1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: #174: feat(orchestration): add content filtering and prompt shield module

Author: lenin-ribeiro Branch: feat/orchestration-filteringmain
Verdict: ⚠️ Needs Minor Work
Summary: Solid, well-structured addition of Azure Content Safety + Prompt Shield to aicore; all CI passes, but a few naming/structure details need attention before merge.


A: Process & Compliance

# Criterion Status Finding
A1 PR template complete ✅ Pass All 9 checklist items ticked; description, type, and how-to-test all present.
A2 Conventional Commits ✅ Pass All 26 commit headlines match type(scope): description. Commit Validation CI job passes. Note: PR title scope is orchestration while the code lives in aicore — cosmetic mismatch but lint still passes.
A3 Issue linked ⚠️ Warning No Closes #N / Fixes #N. PR body says "N/A — new feature proposed and implemented by the App Foundation agent team." Acceptable for an internally-initiated feature, but if any backlog issue exists it should be linked.
A4 AI-generated code disclosure ✅ Pass PR body explicitly discloses Claude/Anthropic assistance and links the SAP AI contribution guideline.

B: Security & Sensitive Data

# Criterion Status Finding
B1 No sensitive data in code ✅ Pass No credentials, tokens, internal URLs, or tenant IDs found in diff. The AICORE_FILTER_TEST_SELF_HARM_PROMPT is explicitly left as an env-var placeholder (never committed) per commit message and integration test.
B2 No sensitive data in PR body ✅ Pass PR body is clean; code examples use generic placeholders.

C: Code Quality

# Criterion Status Finding
C1 CI checks passing ✅ Pass All 11 required jobs pass: Code Quality Checks, Tests & Coverage, Build & Package, Commit Validation, Check Version Bump, Integration Tests, REUSE Compliance, Analyze (python)/CodeQL, license/cla.
C2 Version bump ✅ Pass pyproject.toml bumped 0.27.1 → 0.28.0; uv.lock regenerated.
C3 Type hints ✅ Pass All new public functions and class attributes are fully annotated. from __future__ import annotations used throughout. Internal _install(cfg: Any) uses Any with an explanatory comment to avoid a circular import — acceptable.
C4 No hardcoded values ✅ Pass All severity defaults use Severity.MEDIUM enum; all env-var keys are string constants at call-sites.
C5 Import organization ✅ Pass No lazy imports without justification; no duplicate requirements files.
C6 Naming conventions ⚠️ Warning Two issues: (1) Severity enum members are declared in semantic/severity-ascending order (STRICT=0, LOW=2, MEDIUM=4, OFF=6) rather than alphabetical (LOW, MEDIUM, OFF, STRICT) per guidelines (filters.py:37–48). (2) LlamaGuard38bFilter constructor parameters are in ML-categorization order, not alphabetical — hate and self_harm appear near the bottom rather than sorted lexicographically (filters.py:138–155). Neither affects runtime behavior, but both deviate from the stated convention.
C7 No unused code ✅ Pass No dead imports or unused variables introduced. Stale ty: ignore directives were cleaned up in the last two commits.
C8 No unjustified new dependencies ✅ Pass No new runtime dependencies added; litellm was already present.
C9 Proto code freshness ➖ N/A No .proto files changed.

D: API & Design

# Criterion Status Finding
D1 API future-proofing ✅ Pass Class-based ContentFiltering / InputFiltering / OutputFiltering dataclass-like pattern is well-structured and extensible. Severity IntEnum over bare ints. No create_client() needed — filtering is a configuration helper, not a service client.
D2 Public API hygiene ✅ Pass __all__ in both filtering/__init__.py and aicore/__init__.py expose exactly the documented 13 public symbols. Internal pieces (_litellm_patch, _install, _active_cfg, _ORIGINAL_CONFIG) are correctly prefixed with _. filters.py (no underscore) correctly signals it IS the public surface.
D3 Breaking changes marked ✅ Pass This is an additive new feature. set_aicore_config() gains a side effect (filtering activation), which is a behavioral change but not a signature break. The user guide's Migration section covers the sap_cloud_sdk.orchestrationsap_cloud_sdk.aicore rename for any in-flight consumers.
D4 Pagination & tenant filtering ➖ N/A Not a list/query operation.
D5 Telemetry instrumentation ✅ Pass @record_metrics(Module.AICORE, Operation.AICORE_SET_FILTERING) on set_filtering, AICORE_DISABLE_FILTERING on disable_filtering, AICORE_EXTRACT_FILTER_BLOCKED on extract_filter_blocked. Module.ORCHESTRATION correctly dropped; operation.py updated with the three new operations.

E: Tests & Documentation

# Criterion Status Finding
E1 Tests added/updated ✅ Pass 68 unit tests across test_filtering.py, test_filters.py, test_modules.py, test_patch.py + 22 core/env tests. Integration BDD suite (filtering.feature + test_filtering_bdd.py) covers all 4 live scenarios with clean skip when env absent. tests/aicore/unit/test_aicore.py updated to reflect the new public API.
E2 Documentation quality ✅ Pass aicore/user-guide.md comprehensively updated: overview, quick start, env-var table, programmatic override, multiple providers, disable, error handling, and migration guide.
E3 Module structure compliance ⚠️ Warning (1) py.typed is missing from src/sap_cloud_sdk/aicore/filtering/ — sub-packages of PEP 561 packages need their own marker file. The parent aicore/ also lacks it (pre-existing gap), but the new filtering/ sub-package doesn't add one either. (2) config.py is absent; ContentFiltering.from_env() lives in filters.py — this is a deliberate post-review consolidation decision (review comment #14) and the result is cleaner, but it deviates from the standard layout. Not blocking given the documented rationale. (3) AICORE_FILTER_TEST_MODEL and AICORE_FILTER_TEST_SELF_HARM_PROMPT are not documented in .env_integration_tests.example — other integration test env vars for the aicore module (AICORE_CLIENT_ID, AICORE_MODEL, etc.) are listed there.

⚠️ Non-Blocking Suggestions

  • [C6]: Severity enum members should be in alphabetical order per guidelines: LOW, MEDIUM, OFF, STRICTfilters.py:45–48. The current ordering is semantically meaningful (ascending strictness) but breaks the stated convention.
  • [C6]: LlamaGuard38bFilter constructor parameters should be alphabetical. Current grouping (violent_crimes, non_violent_crimes, sex_crimes, …, hate, self_harm, …) follows ML taxonomy order, not lexicographic — filters.py:141–154.
  • [E3]: Add py.typed to src/sap_cloud_sdk/aicore/filtering/ (empty file). While the parent aicore/ also lacks one, starting the new sub-package correctly is lower-effort here.
  • [E3]: Add AICORE_FILTER_TEST_MODEL and AICORE_FILTER_TEST_SELF_HARM_PROMPT to .env_integration_tests.example alongside the existing AICORE_* vars so contributors know what's needed to run the integration suite.
  • [_litellm_patch.py:211]: The except Exception: return None in extract_filter_blocked is intentionally broad (any JSON parsing or dict-navigation failure should silently return None). Consider narrowing to except (ValueError, KeyError, TypeError) to document the specific failure modes explicitly, even if the behavior is unchanged.

✅ Things Done Well

  • Excellent post-review responsiveness: 14+ review comments from @NicoleMGomes all addressed (collapsing to aicore, removing broad except, Severity enum, class-based API, env helpers, telemetry consolidation). Commit messages are unusually detailed and audit-friendly.
  • Clean monkeypatch design: _ORIGINAL_CONFIG capture + idempotent _install(None) restore is robust and testable; the test fixture restore_litellm ensures no test leaks.
  • Wire-format alignment: Mirroring gen_ai_hub.orchestration.models.content_filtering shapes means callers migrating from generative-ai-hub-sdk keep their call-site structure with minimal diffs.
  • Security-conscious integration test design: AICORE_FILTER_TEST_SELF_HARM_PROMPT stays out of the repo entirely — operator-populated from a secret. Good pattern.
  • Separation of concerns in core/env.py: Typed env-var readers are cleanly separated from secret_resolver (credential loader), with clear docstring justification.
  • ContentFiltering.from_env() returning None on AICORE_FILTER_ENABLED=false threads correctly through set_filtering()_install(None) → restores original LiteLLM config. The logic is airtight.

…ers.py

Address review comments #17 and #18 (part 1):

- Merge _litellm_patch.py (LiteLLM transport patch, _install, _active_cfg,
  _ORIGINAL_CONFIG, FilteringOrchestrationConfig, extract_filter_blocked)
  into filters.py. The whole filtering surface (Severity, ContentFilter,
  AzureContentFilter, LlamaGuard38bFilter, InputFiltering, OutputFiltering,
  ContentFiltering, set_filtering, disable_filtering, extract_filter_blocked,
  FilteringOrchestrationConfig) now lives in one file at ~565 lines.
- Inline _read_env_str / _read_env_bool / _read_env_choice helpers from
  core/env.py into filters.py (private module-level helpers). Delete
  src/sap_cloud_sdk/core/env.py + tests/core/unit/test_env.py — single
  consumer, no need for a shared module yet.

aicore/filtering/__init__.py stays a thin re-export of the public names
plus the exception types. exceptions.py is the only other file in the
package.

Test imports updated: test_filtering.py and test_patch.py now import
_install / _ORIGINAL_CONFIG / FilteringOrchestrationConfig from
.filters (not ._litellm_patch). Mock paths in test_patch.py updated to
match the new module location.
- Add py.typed markers to aicore/ and aicore/filtering/ packages so type
  checkers consuming the SDK see the inline annotations (PEP 561).
- Document AICORE_FILTER_TEST_MODEL and AICORE_FILTER_TEST_SELF_HARM_PROMPT
  in .env_integration_tests.example alongside the existing AICORE_*
  entries, with a comment explaining the self-harm prompt is operator-
  populated and kept out of source.
- Narrow extract_filter_blocked's catch-all 'except Exception' to
  '(ValueError, KeyError, TypeError, AttributeError)'. Documents the
  realistic failure modes (JSON parse, missing key, wrong shape, attr on
  non-object) and stops swallowing logic bugs in ContentFilteredError
  construction.
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.

3 participants