Skip to content

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

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

feat(orchestration): add content filtering and prompt shield module#174
lenin-ribeiro wants to merge 37 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.
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
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
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).
Comment thread src/sap_cloud_sdk/aicore/filtering/_litellm_patch.py
Comment thread src/sap_cloud_sdk/core/env.py Outdated
Comment thread src/sap_cloud_sdk/core/env.py Outdated
Comment thread src/sap_cloud_sdk/aicore/filtering/filters.py

@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.

lenin-ribeiro and others added 7 commits June 23, 2026 14:43
…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.
main advanced to 0.29.0 since we last rebased (commit 101492b on main).
Our 0.28.1 would fail the 'Check Version Bump' CI job which requires
strictly greater than main.

Going to 0.30.0 (minor) rather than 0.29.1 (patch) — this PR adds
substantive new public API (ContentFiltering / InputFiltering /
OutputFiltering / AzureContentFilter / LlamaGuard38bFilter classes;
set_filtering / disable_filtering entry points; Severity enum;
AICORE_FILTER_* env-var protocol). Patch-version connotes bugfix-only,
which understates the surface change.
Address final review comment: 'filters.py file is too bloated. We should
separate concerns, like create a config.py and put the env var fetching
code, and models.py for the classes.'

Match the sibling-module convention (agent_memory, destination, etc.):

- _models.py: Severity enum + provider classes (ContentFilter,
  AzureContentFilter, LlamaGuard38bFilter) + direction containers
  (InputFiltering, OutputFiltering, ContentFiltering). No behaviour
  beyond construction and to_dict() serialisation.
- config.py: load_from_env() function + private _read_env_* helpers.
  Replaces ContentFiltering.from_env() classmethod with a module-level
  factory, matching agent_memory/config.py's load_from_env_or_mount()
  shape.
- _patch.py: FilteringOrchestrationConfig, _install, _active_cfg,
  _ORIGINAL_CONFIG — the LiteLLM transport monkeypatch.
- _api.py: set_filtering, disable_filtering, extract_filter_blocked
  decorated with @record_metrics. Honours the earlier review #12
  ('not good pattern to keep logic on init').
- __init__.py: thin re-export surface (the 12 public names) +
  module-layout doc.

filters.py deleted. Test imports updated to the new module paths;
mock paths in test_patch.py point at _patch.GenAIHubOrchestrationConfig.
ContentFiltering.from_env() classmethod removed in favour of
load_from_env() — callers (tests + _api.py) updated accordingly.
CI's uvx-installed ruff (latest) enforces a tighter line length than
the project's pinned uv-run ruff. One line in load_from_env reformatted
to wrap the Severity(_read_env_choice(...)) call across three lines,
matching the surrounding violence/sexual/self_harm sites.
NicoleMGomes
NicoleMGomes previously approved these changes Jun 24, 2026
Comment thread src/sap_cloud_sdk/aicore/__init__.py
Comment thread src/sap_cloud_sdk/aicore/filtering/_api.py Outdated
…ecate extract_filter_blocked

Addresses two PR #174 review points from Lucas:

1) The public API was defined in _api.py / _models.py — files whose leading
   underscore says 'internal'. The contradiction wasn't earning anything,
   so:

   - aicore/filtering/_api.py    → aicore/filtering/filters.py
   - aicore/filtering/_models.py → aicore/filtering/models.py
   - aicore/filtering/_patch.py  stays underscored (truly internal; the
     subclass it exports is an implementation detail of the monkeypatch).
   - aicore/filtering/config.py and exceptions.py unchanged.
   - tests/aicore/filtering/unit/test_filters.py (which tested models)
     renamed to test_models.py to match what it tests.

2) extract_filter_blocked forced callers to know about litellm's
   raise_for_status() wrapping detail — they had to catch both
   ContentFilteredError (output rejections) AND a generic Exception that
   they then had to feed through extract_filter_blocked (input rejections).

   Add sap_cloud_sdk.aicore.completion and acompletion: thin wrappers
   around litellm.{completion,acompletion} that catch the wrapped
   APIConnectionError and re-raise as ContentFilteredError before it
   reaches caller code. Callers now catch one exception type for 'filter
   blocked you' regardless of direction. extract_filter_blocked is kept
   importable (back-compat) but emits DeprecationWarning and is removed
   from both aicore.__all__ and aicore.filtering.__all__.

   Parsing logic moved to a private _parse_input_filter_error() that the
   wrapper and the deprecated shim both call.

Telemetry: AICORE_COMPLETION and AICORE_ACOMPLETION operations added; the
operation-count test bumps 152 → 154.

User-guide: "Handle blocked requests" rewritten to use aicore.completion
with a single try/except ContentFilteredError block; the
extract_filter_blocked example is replaced with a deprecation note.

Local verification: pytest tests/aicore → 135 passed, 4 skipped;
pytest tests (sans live-credential integration suites) → 2583 passed,
73 skipped; ruff check + ruff format --check + ty check → all green.
Comment thread src/sap_cloud_sdk/aicore/__init__.py Outdated
OutputFiltering,
Severity,
disable_filtering,
extract_filter_blocked, # noqa: F401 — deprecated, kept importable for back-compat

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.

I don't think backward compatibility is needed since it wasn't released yet

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.

I'd not record usage on acompletion and completion since those are just wrappers. Metering could be very verbose because they'd wrap every LLM call and do not reflect adoption

Comment thread src/sap_cloud_sdk/aicore/completion.py Outdated
return blocked if blocked is not None else exc


@record_metrics(Module.AICORE, Operation.AICORE_COMPLETION)

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.

Suggested change
@record_metrics(Module.AICORE, Operation.AICORE_COMPLETION)

Comment thread src/sap_cloud_sdk/aicore/completion.py Outdated
raise translated from exc


@record_metrics(Module.AICORE, Operation.AICORE_ACOMPLETION)

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.

Suggested change
@record_metrics(Module.AICORE, Operation.AICORE_ACOMPLETION)

…on wrappers

Per Lucas's follow-up on PR #174:

- extract_filter_blocked was unreleased — remove the deprecation shim and
  AICORE_EXTRACT_FILTER_BLOCKED enum entry. Integration test_filtering_bdd
  switches to sap_cloud_sdk.aicore.completion.
- completion/acompletion are pass-through wrappers; drop @record_metrics
  (noisy, doesn't reflect adoption) and the related AICORE_COMPLETION /
  AICORE_ACOMPLETION enum entries.

Net: -3 operations (154 → 151).
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.

4 participants