Guardrails: Added config management integration#599
Guardrails: Added config management integration#599rkritika1508 merged 37 commits intoProjectTech4DevAI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoved guardrail configuration from per-request fields into ConfigBlob using a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant JobExec as Job Executor
participant ConfigStore as Config Storage
participant GuardSvc as Guardrails Service
participant ValidatorAPI as Validator API
participant LLM as LLM Provider
Client->>JobExec: Submit LLM request (may reference ConfigBlob)
JobExec->>ConfigStore: resolve_config_blob(config_id)
ConfigStore-->>JobExec: ConfigBlob with Validator refs
JobExec->>GuardSvc: list_validators_config(org_id, project_id, validator_refs)
GuardSvc->>ValidatorAPI: Fetch validator configs
ValidatorAPI-->>GuardSvc: input_validators, output_validators
GuardSvc-->>JobExec: validators split by stage
JobExec->>GuardSvc: run_guardrails_validation(input_text, input_validators, job_id, project_id, org_id)
GuardSvc->>ValidatorAPI: Validate input
ValidatorAPI-->>GuardSvc: input validation result (safe_text/status)
GuardSvc-->>JobExec: input validation response
alt Input validated
JobExec->>LLM: Call LLM with (maybe modified) input
LLM-->>JobExec: LLM response
JobExec->>GuardSvc: run_guardrails_validation(output_text, output_validators, job_id, project_id, org_id)
GuardSvc->>ValidatorAPI: Validate output
ValidatorAPI-->>GuardSvc: output validation result (safe_text/status, rephrase_needed)
GuardSvc-->>JobExec: output validation response
JobExec-->>Client: Final response (safe output or error)
else Validation bypass / error
JobExec-->>Client: Error or bypass response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nishika26
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/app/tests/api/routes/test_llm.py (1)
168-245:⚠️ Potential issue | 🟡 MinorTests no longer exercise guardrails behavior despite their names.
test_llm_call_success_with_guardrailsandtest_llm_call_guardrails_bypassed_still_succeedsare now functionally identical totest_llm_call_success— no guardrails fields in the payload, no guardrails mocking. They no longer test what their names and docstrings claim.Either:
- Update these tests to exercise the new guardrails flow (e.g., using a stored config with
guardrails_config_id, patchingrun_guardrails_validation), or- Remove them to avoid confusion and dead test weight.
backend/app/crud/config/version.py (2)
40-62:⚠️ Potential issue | 🟠 MajorExternal HTTP call before DB commit risks inconsistency.
create_guardrails_validators_if_present(Line 54) makes an HTTP POST to the external guardrails service beforesession.commit()(Line 62). If the commit fails (constraint violation, connection loss, etc.), guardrails validators will already exist externally but noConfigVersionrow references them — creating orphaned data in the guardrails service.Consider reordering: commit the DB record first, then call the external service. If the external call fails, you can roll back or mark the version accordingly. Alternatively, if the external service is idempotent and orphans are acceptable, document this trade-off.
The same pattern exists in
backend/app/crud/config/config.py(Lines 52–57).
145-157:⚠️ Potential issue | 🟡 MinorReturn type
int | Noneis misleading — this always returnsint.
_get_next_versionreturns either1orlatest + 1, neverNone. The return type should beint.Suggested fix
- def _get_next_version(self, config_id: UUID) -> int | None: + def _get_next_version(self, config_id: UUID) -> int:
🤖 Fix all issues with AI agents
In `@backend/app/models/config/version.py`:
- Around line 66-71: The type annotation for guardrails_config_id in
ConfigVersion is incorrect for a nullable DB column; change the annotation from
UUID to UUID | None (or Optional[UUID]) and ensure the Field has a matching
default (e.g., None) so Pydantic accepts missing values while keeping
sa_column_kwargs={"comment": "..."} and nullable=True; update the
guardrails_config_id declaration accordingly to avoid validation errors when
rows lack this value.
In `@backend/app/services/llm/guardrails.py`:
- Line 98: In create_validators_batch, stop logging the entire payload variable
— sanitize it and log only non-sensitive fields (e.g., count, request id,
validator types) or a redacted summary instead of payload to avoid leaking
sensitive validator config/identifiers; also harden handling of the response
stored in data by replacing direct indexing data["data"] with a safe extraction
(use data.get("data") or check "data" in data and validate it's a list/iterable)
and add an error branch that logs the unexpected shape and returns/raises a
controlled error, referencing the create_validators_batch function, the payload
variable, and the data variable.
- Line 98: Replace the root-logger call with the module-level logger: in the
create_validators_batch function replace
logging.info(f"[create_validators_batch] payload: {payload}") with
logger.info(...) so the module-level logger defined as logger (at top of file)
is used for consistent logging configuration and filtering; ensure you use the
exact symbol `logger` and keep the same message format.
- Around line 100-102: The code concatenates settings.KAAPI_GUARDRAILS_URL with
path segments (e.g., the client.post call that posts to
"validators/configs/batch") without normalizing the base URL, which breaks when
KAAPI_GUARDRAILS_URL lacks a trailing slash; update the call in the httpx.Client
block (and any other concatenations of KAAPI_GUARDRAILS_URL) to normalize the
URL first—either ensure settings.KAAPI_GUARDRAILS_URL ends with "/" before
concatenation or use a safe join (e.g., urllib.parse.urljoin) to combine
KAAPI_GUARDRAILS_URL with "validators/configs/batch", matching the pattern used
for the earlier post to KAAPI_GUARDRAILS_URL.
In `@backend/app/tests/crud/config/test_version.py`:
- Around line 55-87: The test hardcodes organization_id=1 when instantiating
ConfigVersionCrud; replace this magic value by deriving the organization id from
the test fixtures or project setup (e.g., obtain the Project's organization_id
via create_test_project or by returning the Project from create_test_config), or
pass None if the parameter is optional and not needed for this mocked path;
update the test to use the real organization id when calling
ConfigVersionCrud(session=db, project_id=config.project_id, config_id=config.id,
organization_id=...) and ensure create_guardrails_validators_if_present is still
patched as before.
🧹 Nitpick comments (9)
backend/app/models/llm/request.py (1)
123-135: Consider adding a stricter schema for guardrail validator entries.
list[dict[str, Any]]is very permissive — any dict is accepted. If guardrail validators have a known structure (e.g., always requiring atypekey), a lightweight Pydantic model or at least amodel_validatorwould catch malformed input at the API boundary rather than at runtime in the guardrails service.backend/app/api/routes/config/config.py (1)
36-38:organization_idpassed only increate_config— other mutating endpoints may need it too.Only
create_configpassesorganization_idtoConfigCrud. If any future operation (e.g., update with version creation) needs it, those endpoints will silently getNone. This is safe today sinceConfigCrud.__init__defaultsorganization_idtoNone, but worth keeping in mind for consistency.backend/app/api/routes/config/version.py (1)
25-42: Missing return type hints on route handler functions.All four route handlers (
create_version,list_versions,get_version,delete_version) lack return type annotations. As per coding guidelines, all function parameters and return values should have type hints.Example fix for create_version
def create_version( config_id: UUID, version_create: ConfigVersionCreate, current_user: AuthContextDep, session: SessionDep, -): +) -> APIResponse[ConfigVersionPublic]:backend/app/tests/services/llm/test_guardrails.py (1)
141-150: Consider adding a test forcreate_guardrails_validators_if_presentwhen guardrails are present.The current test only covers the no-op path (guardrails=None). A companion test verifying that
create_validators_batchis called with correctly staged validators when guardrails data is provided would strengthen coverage of the happy path.backend/app/crud/config/config.py (1)
3-3: Use built-intupleinstead of deprecatedtyping.Tuple.Per static analysis (Ruff UP035) and Python 3.11+ conventions,
typing.Tupleis deprecated.Suggested fix
-from typing import Optional, Tuple +from typing import OptionalAnd update the return type on Line 33:
- ) -> Tuple[Config, ConfigVersion]: + ) -> tuple[Config, ConfigVersion]:backend/app/services/llm/jobs.py (3)
316-316: Useis Trueor a truthiness check instead of== True.
== Trueis a common anti-pattern in Python — it can match truthy non-bool values unexpectedly. Preferis Truefor strict bool comparison or just a truthiness check.Suggested fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]:
231-240: Unnecessary mutation ofrequest.query.inputbefore returning error.Line 232 sets
request.query.input = safe_input["error"]but the code immediately returns viahandle_job_error. This mutation has no effect and is confusing — it looks like the input is being replaced with an error string.Suggested fix
else: - request.query.input = safe_input["error"] - callback_response = APIResponse.failure_response( error=safe_input["error"], metadata=request.request_metadata,
324-326: Same unnecessary input mutation on output guardrails failure path.Line 326 sets
response.response.output.text = safe_output["error"]then immediately returns an error. This mutates the response object unnecessarily before discarding it.Suggested fix
else: - response.response.output.text = safe_output["error"] - callback_response = APIResponse.failure_response( error=safe_output["error"],backend/app/services/llm/guardrails.py (1)
86-96: Redundant type annotation and None check.Line 88 declares a type annotation for
payloadthat is immediately overwritten. Line 94's ternarystr(config_id) if config_id is not None else Noneis unreachable for theNonebranch since Line 90 already raises onNone.Suggested fix
try: - payload: dict[str, Any] | list[dict] - if config_id is None: raise ValueError("config_id must be provided") payload = { - "config_id": str(config_id) if config_id is not None else None, + "config_id": str(config_id), "validators": validators, }
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/guardrails.py`:
- Around line 206-209: The loop that builds the validators list uses {"stage":
"input", **validator} and {"stage": "output", **validator}, but spreading
validator after the explicit "stage" allows a validator's own "stage" to
override the intended value; update the aggregation so the explicit stage key
comes after the spread (e.g., {**validator, "stage": "input"} and {**validator,
"stage": "output"}) when appending to validators, ensuring the enforced "stage"
always wins for entries from guardrails.input and guardrails.output.
🧹 Nitpick comments (4)
backend/app/services/llm/guardrails.py (4)
13-14: Minor type hint inconsistency:list[dict]→list[dict[str, Any]].Other functions in this file use
list[dict[str, Any]]for the same kind of parameter. Keeping it consistent here improves readability. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code".Suggested fix
def run_guardrails_validation( - input_text: str, guardrail_config: list[dict], job_id: UUID + input_text: str, guardrail_config: list[dict[str, Any]], job_id: UUID ) -> dict[str, Any]:
88-96: Dead code: theelse Nonebranch on line 94 and thelist[dict]type on line 88 are unreachable.Line 91 raises
ValueErrorwhenconfig_id is None, so by line 94,config_idis always non-None. The ternary'selse Noneis dead, and thelist[dict]union arm in the type annotation is never used.Simplify
- payload: dict[str, Any] | list[dict] - - if config_id is None: - raise ValueError("config_id must be provided") - - payload = { - "config_id": str(config_id) if config_id is not None else None, + if config_id is None: + raise ValueError("config_id must be provided") + + payload: dict[str, Any] = { + "config_id": str(config_id),
214-229: Consider adding a brief docstring and an entry log for traceability.This is a public orchestration function called from outside the module. A docstring would help future readers, and an entry-level log line (consistent with the coding guidelines'
[function_name]prefix) would aid debugging in production. As per coding guidelines, "Prefix all log messages with the function name in square brackets".
81-85: Duplicated header construction across three functions.The same
headersdict (withaccept,Authorization,Content-Type) is built inrun_guardrails_validation,create_validators_batch, andget_validators_config. Consider extracting a shared helper like_guardrails_headers()to reduce duplication.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
309-309:⚠️ Potential issue | 🟡 MinorUse truthiness check instead of
== True— inconsistent with the input path.Line 216 uses
if safe_input["data"]["rephrase_needed"]:but line 309 uses== True. This is inconsistent and== Trueis not idiomatic Python.Proposed fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]:
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/guardrails.py`:
- Around line 208-214: The error log in the except block of
get_validators_config currently prints the full validator_configs which may leak
sensitive data; change the log to avoid full dumps by logging only non-sensitive
metadata (e.g., validator count and/or a list of validator IDs) and apply
mask_string(...) to any values considered sensitive before logging (for example,
replace f"validator_configs={validator_configs}" with something like
f"validator_count={len(validator_configs)}" and/or
f"validator_ids={[mask_string(id) for id in ids]}"); keep the rest of the
context (organization_id, project_id, endpoint, error) as-is.
- Around line 92-118: The code inconsistently treats
settings.KAAPI_GUARDRAILS_URL as both a full endpoint and a base URL (affecting
run_guardrails_validation, create_validators_batch, and get_validators_config);
choose one consistent approach—preferably introduce a base setting (e.g.,
KAAPI_GUARDRAILS_BASE_URL) and update run_guardrails_validation to post to
f"{KAAPI_GUARDRAILS_BASE_URL}/<validation-path>" (or split into
KAAPI_GUARDRAILS_VALIDATION_URL and KAAPI_GUARDRAILS_BASE_URL if you want
separate endpoints) and update create_validators_batch and get_validators_config
to append their paths to that base; also remove the redundant if config_id is
not None check in create_validators_batch (config_id is ensured by the earlier
ValueError) and simplify the payload type annotation by dropping the unused
list[dict] branch and using a single dict[str, Any] for payload.
🧹 Nitpick comments (7)
backend/app/crud/config/config.py (1)
3-3: Use built-intupleinstead oftyping.Tuple(deprecated in Python 3.9+).Since the project targets Python 3.11+, prefer the built-in
tupleand drop thetypingimport forTuple. Also,Optional[int]on line 25 is inconsistent with theX | Nonepattern used elsewhere in the codebase (e.g., line 81).Proposed fix
-from typing import Optional, Tuple +from typing import AnyAnd on line 25:
- def __init__( - self, session: Session, project_id: int, organization_id: Optional[int] = None - ): + def __init__( + self, session: Session, project_id: int, organization_id: int | None = None + ):And update return type on line 31–33:
def create_or_raise( self, config_create: ConfigCreate - ) -> Tuple[Config, ConfigVersion]: + ) -> tuple[Config, ConfigVersion]:As per coding guidelines: "Use Python 3.11+ with type hints throughout the codebase."
backend/app/models/llm/request.py (1)
122-123: Consider a more descriptive field description.The description
"Validator config"is quite terse. A slightly more descriptive text like"ID of the guardrails validator configuration"would improve API docs clarity.Suggested improvement
class Validator(SQLModel): - validator_config_id: int = Field(..., description="Validator config") + validator_config_id: int = Field( + ..., description="ID of the guardrails validator configuration" + )backend/app/services/llm/jobs.py (2)
186-233: No error handling specific toget_validators_configfailures.If
get_validators_config(line 193) raises an exception (e.g., the guardrails service is down), it falls through to the catch-all on line 356, which returns a generic "Unexpected error occurred" message. Consider catching this explicitly to return a more actionable error message, e.g., "Failed to fetch guardrails validator configurations."Proposed improvement
if validator_configs: - input_guardrails, output_guardrails = get_validators_config( - validator_configs=validator_configs, - organization_id=organization_id, - project_id=project_id, - ) + try: + input_guardrails, output_guardrails = get_validators_config( + validator_configs=validator_configs, + organization_id=organization_id, + project_id=project_id, + ) + except Exception as e: + logger.error( + f"[execute_job] Failed to fetch validator configs | job_id={job_id}, error={str(e)}", + exc_info=True, + ) + callback_response = APIResponse.failure_response( + error="Failed to fetch guardrails validator configurations", + metadata=request.request_metadata, + ) + return handle_job_error( + job_id, request.callback_url, callback_response + )
147-148: Tighten the type annotation for guardrails lists.
list[dict]is missing the value type. For consistency with the rest of the codebase and coding guidelines requiring full type hints:Proposed fix
- input_guardrails: list[dict] = [] - output_guardrails: list[dict] = [] + input_guardrails: list[dict[str, Any]] = [] + output_guardrails: list[dict[str, Any]] = []This requires adding
Anyto the typing imports (already available fromapp.models.llm.requestcontext, but would needfrom typing import Anyat the top of the file).As per coding guidelines: "Always add type hints to all function parameters and return values in Python code."
backend/app/services/llm/guardrails.py (3)
93-99: Dead code: redundantNonecheck and unused type branch.Line 93 declares
payloadasdict[str, Any] | list[dict], but thelist[dict]branch is never assigned. Line 99's ternaryif config_id is not None else Noneis unreachable since Line 96 already raises onNone.Suggested cleanup
- payload: dict[str, Any] | list[dict] - - if config_id is None: - raise ValueError("config_id must be provided") - - payload = { - "config_id": str(config_id) if config_id is not None else None, + if config_id is None: + raise ValueError("config_id must be provided") + + payload = { + "config_id": str(config_id),
193-206: Validators with unrecognizedstagevalues are silently dropped.If a validator has a
stageother than"input"or"output"(e.g., a typo like"inpt"or a new stage added later), it will be silently excluded from both lists. This could mask misconfiguration or data issues.Consider logging a warning for unrecognized stages:
Suggested improvement
input_guardrails = [ validator for validator in validators if isinstance(validator, dict) and str(validator.get("stage", "")).lower() == "input" ] output_guardrails = [ validator for validator in validators if isinstance(validator, dict) and str(validator.get("stage", "")).lower() == "output" ] + recognized_count = len(input_guardrails) + len(output_guardrails) + if recognized_count < len(validators): + logger.warning( + f"[get_validators_config] {len(validators) - recognized_count} " + "validator(s) had unrecognized stage values and were dropped." + ) + return input_guardrails, output_guardrails
86-90: Consider extracting shared headers into a helper to reduce duplication.The same header block (
accept,Authorization,Content-Type) is repeated in all three functions (Lines 38–42, 86–90, 157–161). A small helper would reduce duplication and ensure consistency if the auth scheme changes.def _guardrails_headers() -> dict[str, str]: return { "accept": "application/json", "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}", "Content-Type": "application/json", }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/guardrails.py`:
- Around line 91-100: The HTTP client is sending query params with None values
which httpx renders as the string "None"; before calling httpx.Client.post build
the params dict for the POST (the one currently constructed with
"organization_id": organization_id and "project_id": project_id) and remove any
entries whose value is None (e.g., filter params = {k: v for k, v in
params.items() if v is not None}) so only present keys are passed to
client.post; update the call that uses validator_configs and headers to use this
filtered params dict.
🧹 Nitpick comments (1)
backend/app/services/llm/guardrails.py (1)
118-129: Validators with stages other than"input"/"output"are silently discarded.If the API returns validators with an unexpected or missing
stagevalue (e.g.,"pre-processing","", orNone), they are silently dropped by both list comprehensions. Consider logging a warning for unrecognized stages to aid debugging.Suggested addition
+ recognized = set() + for v in validators: + if isinstance(v, dict): + stage = str(v.get("stage", "")).lower() + if stage in ("input", "output"): + recognized.add(id(v)) + else: + logger.warning( + f"[get_validators_config] Ignoring validator with unrecognized stage={v.get('stage')!r}" + ) + input_guardrails = [ validator for validator in validators if isinstance(validator, dict) and str(validator.get("stage", "")).lower() == "input" ] output_guardrails = [ validator for validator in validators if isinstance(validator, dict) and str(validator.get("stage", "")).lower() == "output" ]
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
285-287:⚠️ Potential issue | 🟠 Major
resolved_config_blobdrops guardrail references.Line 287 constructs a new
ConfigBlob(completion=completion_config)without carrying overinput_guardrailsandoutput_guardrailsfrom the originalconfig_blob. Ifcreate_llm_callpersists this blob, the stored record will lose the guardrail configuration, breaking auditability.Proposed fix
- resolved_config_blob = ConfigBlob(completion=completion_config) + resolved_config_blob = ConfigBlob( + completion=completion_config, + input_guardrails=config_blob.input_guardrails, + output_guardrails=config_blob.output_guardrails, + )
🤖 Fix all issues with AI agents
In `@backend/app/models/llm/request.py`:
- Around line 216-225: The input_guardrails and output_guardrails fields are
typed as list[UUID] but are actually passed lists of dicts like
[{"validator_config_id": 1}], causing validation errors; define a small Pydantic
model (e.g., ValidatorConfigRef with a field validator_config_id: UUID | int)
and change the types of input_guardrails and output_guardrails to
list[ValidatorConfigRef] | None (and update their Field descriptions
accordingly), or alternatively make them list[dict] | None if you want to accept
arbitrary dicts—ensure the new model name is imported/used where
input_guardrails and output_guardrails are declared so runtime consumers (tests
and jobs.py usages) validate correctly.
In `@backend/app/services/llm/jobs.py`:
- Line 385: Replace the explicit equality check `if
safe_output["data"]["rephrase_needed"] == True:` with a truthiness check `if
safe_output["data"]["rephrase_needed"]:`; update the conditional in the jobs.py
code path that evaluates safe_output["data"]["rephrase_needed"] (where the
rephrase branch is handled) to remove the redundant `== True` per Ruff E712.
- Around line 209-220: The call to get_validators_config can raise and currently
is uncaught; wrap the call in a try/except around the block that uses
config_blob (where input_guardrails and output_guardrails are constructed) to
catch exceptions from get_validators_config, log a clear guardrails-specific
error (including the exception details and validator_config_ids), and then set
input_guardrails and output_guardrails to safe defaults (e.g., empty lists) or
skip validator processing so the job continues rather than crashing; reference
the get_validators_config call and the variables validator_config_ids,
input_guardrails, output_guardrails in your change.
- Around line 222-256: The code is passing a QueryInput Pydantic model
(request.query.input) into run_guardrails_validation which expects a plain str
and then overwrites request.query.input with a str; instead extract the actual
text (e.g., text_value = request.query.input.content.value or use the existing
helper that resolve_input returns), call run_guardrails_validation(text_value,
input_guardrails, job_id), and when applying results re-wrap the safe text back
into the same QueryInput subclass (e.g., construct a new TextInput or
copy/update the existing request.query.input with the safe_text in its content
field) so downstream code (resolve_input, any consumers of
request.query.input.content) still see a QueryInput object; keep the bypassed
and error branches using APIResponse.failure_response and handle_job_error as
before but do not assign plain strings to request.query.input.
In `@backend/app/tests/api/routes/configs/test_config.py`:
- Around line 62-96: The ConfigBlob type is wrong for guardrails: tests (and
resolve_config_blob in jobs.py) expect each guardrail item to be a Pydantic
model/dict with a validator_config_id, not bare UUIDs; update the model in
backend/app/models/llm/request.py by introducing a small Pydantic model (e.g.,
ValidatorRef or ValidatorConfigRef with field validator_config_id: UUID) and
change ConfigBlob.input_guardrails and ConfigBlob.output_guardrails to use
list[ValidatorRef] | None, then adjust any places that construct or validate
ConfigBlob (resolve_config_blob in jobs.py and any callers) so they
accept/instantiate the new model shape. Ensure imports and type hints are
updated accordingly.
In `@backend/app/tests/crud/config/test_config.py`:
- Around line 62-87: The test is passing dicts for guardrails but
ConfigBlob.input_guardrails/output_guardrails expect list[UUID]; update
test_create_config_with_guardrails_persists_validator_refs to supply UUIDs
instead of dicts—e.g., create or retrieve validator records and use their id
UUIDs (or construct proper uuid.UUID values) when building ConfigBlob in
ConfigCreate so ConfigCrud.create_or_raise and ConfigBlob validation succeed
(refer to ConfigBlob, ConfigCreate, ConfigCrud, and the test function name to
locate the change).
In `@backend/app/tests/crud/config/test_version.py`:
- Around line 54-76: The test
test_create_version_with_guardrails_persists_validator_refs fails because
ConfigBlob.model_validate expects validator_config_id to be a UUID but the test
passes integers; update the test to pass valid UUIDs (e.g., generate or fetch
actual validator_config.id values or use uuid4() strings) in the config_blob
input_guardrails/output_guardrails instead of 1 and 2 so
ConfigVersionCrud.create_or_raise (which calls ConfigBlob.model_validate) can
parse them successfully.
In `@backend/app/tests/services/llm/test_jobs.py`:
- Around line 960-988: The test
test_execute_job_fetches_validator_configs_from_blob_refs fails because the
request config omits the required NativeCompletionConfig "type" field, it
asserts the wrong keyword from get_validators_config, and it treats returned IDs
as objects. Fix by adding the required "type" (e.g., "text") under
request_data["config"]["blob"]["completion"] so
LLMCallRequest/NativeCompletionConfig validation passes; update the post-patch
assertion to read the actual kwarg name used in jobs.py (use
kwargs["validator_config_ids"] instead of kwargs["validator_configs"]) when
inspecting mock_fetch_configs.call_args; and compare the returned list directly
to UUIDs (e.g., assert list(kwargs["validator_config_ids"]) == [UUID(1),
UUID(2)] or the literal UUID values) rather than accessing a
.validator_config_id attribute.
- Around line 1021-1048: The test is failing because NativeCompletionConfig is
missing its required type field and the assertions call .model_dump() on UUIDs;
update the test test_resolve_config_blob_keeps_validator_refs so when
constructing NativeCompletionConfig (in the config_blob) include the required
type (e.g., type="completion" or the correct enum/string expected by
NativeCompletionConfig), and change the assertions for
resolved_blob.input_guardrails and resolved_blob.output_guardrails to not call
.model_dump() on UUID values but instead compare the UUIDs/ids directly (for
example by mapping to str(v) or comparing to UUID objects) so the expected
[{"validator_config_id": 1}] is matched appropriately. Ensure you adjust
LLMCallConfig/ConfigBlob usage around resolve_config_blob if needed to reflect
the type addition.
🧹 Nitpick comments (1)
backend/app/services/llm/jobs.py (1)
154-155: Missing generic type argument in type hints.Per coding guidelines, use full type hints.
list[dict]should belist[dict[str, Any]].Proposed fix
- input_guardrails: list[dict] = [] - output_guardrails: list[dict] = [] + input_guardrails: list[dict[str, Any]] = [] + output_guardrails: list[dict[str, Any]] = []
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
402-409:⚠️ Potential issue | 🔴 CriticalTwo issues:
== Truecomparison anderror=request.query.inputpasses aQueryInputobject as the error string.
- Line 402:
== Trueflagged by Ruff E712; use truthiness check.- Line 404:
error=request.query.input—request.query.inputis aQueryInput(Pydantic model), not a string. This produces a non-human-readable error message. It should likely be the output text or a descriptive message like"Output failed guardrail validation: rephrase needed".Proposed fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]: callback_response = APIResponse.failure_response( - error=request.query.input, + error="Output guardrail validation failed: rephrase needed", metadata=request.request_metadata, )
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/jobs.py`:
- Around line 24-31: Remove the duplicate import block in
backend/app/services/llm/jobs.py: keep a single import of list_validators_config
and run_guardrails_validation from app.services.llm.guardrails and delete the
redundant second import statement so the file only imports those symbols once.
In `@backend/app/tests/services/llm/test_guardrails.py`:
- Around line 78-91: The test named
test_run_guardrails_validation_serializes_validator_models doesn't exercise the
Validator branch because it passes a plain dict; update the test to construct
and pass a Validator instance (import Validator from the module that defines it)
to run_guardrails_validation so the isinstance(validator, Validator) branch in
guardrails.run_guardrails_validation is exercised and serialized; keep the
existing mock_client setup and assert that mock_client.post was called with
kwargs["json"]["validators"] equal to the expected serialized dict
representation of that Validator instance.
🧹 Nitpick comments (3)
backend/app/services/llm/guardrails.py (2)
90-94:Content-Typeheader is unnecessary on a GET request with no body.The
Content-Type: application/jsonheader is semantically meaningless for a GET request and may confuse some servers or proxies.Proposed fix
headers = { "accept": "application/json", "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}", - "Content-Type": "application/json", }
13-17: Broadenedguardrail_configtype may mask misuse.The parameter
guardrail_config: list[Validator | dict[str, Any]]accepts bothValidatorobjects and raw dicts. This is a pragmatic choice during migration, but the callers injobs.py(lines 233, 384) always passlist[dict[str, Any]](the output oflist_validators_config). Consider narrowing the type tolist[dict[str, Any]]once migration is complete, or validating dict shapes inside the function.backend/app/tests/api/routes/test_llm.py (1)
170-208: Test docstrings are stale — they reference guardrails behavior that is no longer tested here.
test_llm_call_success_with_guardrails(line 174: "no validators") andtest_llm_call_guardrails_bypassed_still_succeeds(line 215) no longer mock or assert any guardrails behavior. The tests now just verify that the endpoint accepts requests and starts a job — effectively duplicating earlier tests.Consider updating the docstrings to reflect the actual test intent, or removing these tests if the coverage is now redundant with
test_llm_call_success.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/guardrails.py`:
- Around line 99-111: The params dict passed to httpx.Client().get in
guardrails.py can contain organization_id or project_id set to None which httpx
will serialize as the string "None"; update the code around the
httpx.Client(...) call (the block that builds params for endpoint) to remove
keys whose values are None (keep ids/validator_config_ids intact) before calling
client.get so only present parameters are sent to the downstream API.
🧹 Nitpick comments (2)
backend/app/services/llm/jobs.py (2)
402-402: Avoid== Truecomparison; use truthiness check.This is flagged by Ruff E712.
♻️ Proposed fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]:
219-230: Consider adding explicit error handling forlist_validators_configfailures.If
list_validators_configraises an exception (e.g., network failure, invalid response), it will propagate to the outer catch-all (line 470), producing a generic "Unexpected error occurred" message instead of a guardrail-specific error.♻️ Proposed improvement
if config_blob is not None: validator_configs: list[Validator] = [ *(config_blob.input_guardrails or []), *(config_blob.output_guardrails or []), ] if validator_configs: - input_guardrails, output_guardrails = list_validators_config( - organization_id=organization_id, - project_id=project_id, - validator_configs=validator_configs, - ) + try: + input_guardrails, output_guardrails = list_validators_config( + organization_id=organization_id, + project_id=project_id, + validator_configs=validator_configs, + ) + except Exception as e: + logger.error( + f"[execute_job] Failed to fetch validator configs: {e} | job_id={job_id}" + ) + callback_response = APIResponse.failure_response( + error="Failed to fetch guardrail configurations", + metadata=request.request_metadata, + ) + return handle_job_error( + job_id, request.callback_url, callback_response + )
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
396-403:⚠️ Potential issue | 🔴 Critical
request.query.inputpassed as error string for output rephrase — this is aQueryInputobject, notstr.Two issues at line 398:
request.query.inputis aQueryInputPydantic model, not a string. Passing it toAPIResponse.failure_response(error=...)which expectsstr | listwill produce a repr of the object as the error message.- Semantically, for an output guardrail rephrase, using the original input as the error doesn't make sense. It should use the output safe_text, consistent with how input guardrails handle rephrase at line 245.
Proposed fix
if safe_output["data"]["rephrase_needed"] == True: callback_response = APIResponse.failure_response( - error=request.query.input, + error=safe_output["data"]["safe_text"], metadata=request.request_metadata, )
🤖 Fix all issues with AI agents
In `@backend/app/services/llm/guardrails.py`:
- Around line 71-75: list_validators_config can raise while
run_guardrails_validation swallows errors; to avoid surprising callers, wrap the
call to list_validators_config inside execute_job with a targeted try/except
that catches the specific exceptions list_validators_config can raise (or
Exception as a fallback), log the error, and convert it into the guardrails
fail-open behavior (e.g., return the same bypass/allow response shape
run_guardrails_validation uses) or re-raise a GuardrailsValidationError; update
execute_job (in jobs.py) to call list_validators_config inside this try/except
so guardrail failures produce a consistent, documented outcome rather than
leaking generic "Unexpected error occurred".
🧹 Nitpick comments (2)
backend/app/tests/services/llm/test_guardrails.py (1)
97-136: Consider adding a test forlist_validators_configerror propagation.The happy path and empty-input short-circuit are covered, but there's no test verifying that an HTTP error from the external service propagates as an exception (since
list_validators_configre-raises). This matters becauseexecute_jobneeds to handle that failure.Would you like me to generate a test for the error path?
backend/app/services/llm/jobs.py (1)
251-261: Unnecessary mutation ofrequest.query.input.content.valuebefore returning error.Line 253 sets
request.query.input.content.value = safe_input["error"], but then immediately returns viahandle_job_error. The mutation has no downstream effect since the request is abandoned. The same pattern appears at line 406 for output guardrails. Consider removing these mutations for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
406-413:⚠️ Potential issue | 🔴 CriticalBug:
request.query.inputis aQueryInputobject, not astr— the error message will be garbled.
APIResponse.failure_response(error=...)expectsstr | list[dict], butrequest.query.inputis a Pydantic model (QueryInput). This will serialize as the model'srepr()string, producing an unusable error message for the callback consumer.Additionally,
== Trueshould be a truthiness check per Ruff E712 (flagged in a prior review).Proposed fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]: callback_response = APIResponse.failure_response( - error=request.query.input, + error=safe_output["data"]["safe_text"], metadata=request.request_metadata, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 406 - 413, The code passes a Pydantic QueryInput object to APIResponse.failure_response and uses an explicit == True check; update the branch so it checks truthiness (if safe_output["data"]["rephrase_needed"]: ) and pass a human-readable string extracted from request.query.input (for example request.query.input.text or another appropriate text field on QueryInput) to APIResponse.failure_response instead of the model itself; keep the existing callback via handle_job_error(job_id, request.callback_url, callback_response).
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/llm/guardrails.py`:
- Around line 138-149: The comprehensions that build input_guardrails and
output_guardrails silently ignore any validators that are not dicts (validators
variable), which can hide unexpected API responses; modify the logic that
constructs input_guardrails and output_guardrails to detect non-dict entries in
validators, log a warning via the module logger (or existing logger) including
the offending item and its index, and still skip non-dict entries so behavior is
unchanged; locate the comprehensions that create input_guardrails and
output_guardrails and add a small loop or comprehension with conditional that
records a warning when isinstance(validator, dict) is false, referencing the
validators variable and the input_guardrails/output_guardrails names.
In `@backend/app/services/llm/jobs.py`:
- Around line 219-224: The call to list_validators_config (which populates
input_guardrails and output_guardrails using organization_id, project_id,
validator_configs) can raise and currently bubbles up producing a generic error;
wrap this call in a try/except that catches exceptions from
list_validators_config, log a clear guardrail-specific error message (including
the caught exception) and either re-raise a more specific GuardrailError or set
input_guardrails/output_guardrails to safe defaults (e.g., empty lists) so the
job can continue with a meaningful error message instead of the generic
"Unexpected error occurred".
- Around line 156-160: The annotations for input_guardrails and
output_guardrails are too broad (list[dict]) and must be changed to
list[dict[str, Any]] to match the actual values and the return type of
list_validators_config; import Any from typing (or add it to the existing typing
import) and update the variable annotations for input_guardrails and
output_guardrails in jobs.py to use dict[str, Any], ensuring consistency with
list_validators_config.
- Around line 245-266: The code is performing dead writes to
request.query.input.content.value before immediately returning on error paths;
remove the unnecessary mutations by only assigning
request.query.input.content.value when the safe_input is successful and rephrase
is not needed (i.e., in the success branch after confirming rephrase_needed is
false), and likewise avoid setting it in the failure branch before constructing
the APIResponse.failure_response; keep the use of APIResponse.failure_response,
handle_job_error, job_id, request.callback_url, and request.request_metadata
as-is, just move or remove the two assignments to avoid overwriting the request
when the function returns immediately.
- Around line 138-145: The execute_job function signature is missing a type hint
for the task_instance parameter; update the signature in
execute_job(request_data: dict, project_id: int, organization_id: int, job_id:
str, task_id: str, task_instance) -> dict to include an explicit type for
task_instance (prefer the concrete type used elsewhere in the codebase, e.g.,
TaskInstance or Celery Task/TaskInstance if applicable); if the exact type is
not available, import and use typing.Any (from typing import Any) and annotate
task_instance: Any, keeping the return type as dict.
- Line 267: Remove the unused local variable assignment
user_sent_config_provider (assigned to an empty string) from the function where
it appears; delete the line initializing user_sent_config_provider, ensure no
remaining references exist, and run linters/tests to confirm no regressions.
- Around line 406-413: The code passes a Pydantic QueryInput object to
APIResponse.failure_response and uses an explicit == True check; update the
branch so it checks truthiness (if safe_output["data"]["rephrase_needed"]: ) and
pass a human-readable string extracted from request.query.input (for example
request.query.input.text or another appropriate text field on QueryInput) to
APIResponse.failure_response instead of the model itself; keep the existing
callback via handle_job_error(job_id, request.callback_url, callback_response).
In `@backend/app/tests/services/llm/test_guardrails.py`:
- Around line 191-203: Add a new unit test for list_validators_config that
verifies it propagates or re-raises HTTP errors when the external API call
fails: patch app.services.llm.guardrails.httpx.Client (same as
test_list_validators_config_empty_short_circuits_without_http), configure the
mocked client's get() response to raise on raise_for_status (or to return a
response whose raise_for_status side effect raises an httpx.HTTPStatusError),
call list_validators_config with a non-empty validator_configs so the function
does not short-circuit, and assert that the HTTP error is raised and that the
mocked client was called; reference list_validators_config, mock_client_cls, and
raise_for_status in your test.
🧹 Nitpick comments (6)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py`: - Around line 138-149: The comprehensions that build input_guardrails and output_guardrails silently ignore any validators that are not dicts (validators variable), which can hide unexpected API responses; modify the logic that constructs input_guardrails and output_guardrails to detect non-dict entries in validators, log a warning via the module logger (or existing logger) including the offending item and its index, and still skip non-dict entries so behavior is unchanged; locate the comprehensions that create input_guardrails and output_guardrails and add a small loop or comprehension with conditional that records a warning when isinstance(validator, dict) is false, referencing the validators variable and the input_guardrails/output_guardrails names. In `@backend/app/services/llm/jobs.py`: - Around line 156-160: The annotations for input_guardrails and output_guardrails are too broad (list[dict]) and must be changed to list[dict[str, Any]] to match the actual values and the return type of list_validators_config; import Any from typing (or add it to the existing typing import) and update the variable annotations for input_guardrails and output_guardrails in jobs.py to use dict[str, Any], ensuring consistency with list_validators_config. - Around line 245-266: The code is performing dead writes to request.query.input.content.value before immediately returning on error paths; remove the unnecessary mutations by only assigning request.query.input.content.value when the safe_input is successful and rephrase is not needed (i.e., in the success branch after confirming rephrase_needed is false), and likewise avoid setting it in the failure branch before constructing the APIResponse.failure_response; keep the use of APIResponse.failure_response, handle_job_error, job_id, request.callback_url, and request.request_metadata as-is, just move or remove the two assignments to avoid overwriting the request when the function returns immediately. - Around line 138-145: The execute_job function signature is missing a type hint for the task_instance parameter; update the signature in execute_job(request_data: dict, project_id: int, organization_id: int, job_id: str, task_id: str, task_instance) -> dict to include an explicit type for task_instance (prefer the concrete type used elsewhere in the codebase, e.g., TaskInstance or Celery Task/TaskInstance if applicable); if the exact type is not available, import and use typing.Any (from typing import Any) and annotate task_instance: Any, keeping the return type as dict. - Line 267: Remove the unused local variable assignment user_sent_config_provider (assigned to an empty string) from the function where it appears; delete the line initializing user_sent_config_provider, ensure no remaining references exist, and run linters/tests to confirm no regressions. In `@backend/app/tests/services/llm/test_guardrails.py`: - Around line 191-203: Add a new unit test for list_validators_config that verifies it propagates or re-raises HTTP errors when the external API call fails: patch app.services.llm.guardrails.httpx.Client (same as test_list_validators_config_empty_short_circuits_without_http), configure the mocked client's get() response to raise on raise_for_status (or to return a response whose raise_for_status side effect raises an httpx.HTTPStatusError), call list_validators_config with a non-empty validator_configs so the function does not short-circuit, and assert that the HTTP error is raised and that the mocked client was called; reference list_validators_config, mock_client_cls, and raise_for_status in your test.backend/app/services/llm/jobs.py (4)
156-160: Type annotations forinput_guardrailsandoutput_guardrailsare incomplete.These are typed as
list[dict]but the values arelist[dict[str, Any]]. More importantly,list_validators_configreturnstuple[list[dict[str, Any]], list[dict[str, Any]]]. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code."Proposed fix
- input_guardrails: list[dict] = [] - output_guardrails: list[dict] = [] + input_guardrails: list[dict[str, Any]] = [] + output_guardrails: list[dict[str, Any]] = []This requires adding
Anyto the existingtypingimports or addingfrom typing import Any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 156 - 160, The annotations for input_guardrails and output_guardrails are too broad (list[dict]) and must be changed to list[dict[str, Any]] to match the actual values and the return type of list_validators_config; import Any from typing (or add it to the existing typing import) and update the variable annotations for input_guardrails and output_guardrails in jobs.py to use dict[str, Any], ensuring consistency with list_validators_config.
245-266: Unnecessary mutation at Line 246 before early-return onrephrase_needed.Line 246 overwrites
request.query.input.content.valuewithsafe_text, but ifrephrase_neededis true (line 248), the function returns an error immediately — the overwritten value is never used. Similarly, Line 258 overwrites with the error string before returning the error. These mutations are dead writes.Proposed simplification
elif safe_input["success"]: - request.query.input.content.value = safe_input["data"]["safe_text"] - if safe_input["data"]["rephrase_needed"]: callback_response = APIResponse.failure_response( error=safe_input["data"]["safe_text"], metadata=request.request_metadata, ) return handle_job_error( job_id, request.callback_url, callback_response ) + request.query.input.content.value = safe_input["data"]["safe_text"] else: - # Update the text value with error message - request.query.input.content.value = safe_input["error"] - callback_response = APIResponse.failure_response( error=safe_input["error"], metadata=request.request_metadata,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 245 - 266, The code is performing dead writes to request.query.input.content.value before immediately returning on error paths; remove the unnecessary mutations by only assigning request.query.input.content.value when the safe_input is successful and rephrase is not needed (i.e., in the success branch after confirming rephrase_needed is false), and likewise avoid setting it in the failure branch before constructing the APIResponse.failure_response; keep the use of APIResponse.failure_response, handle_job_error, job_id, request.callback_url, and request.request_metadata as-is, just move or remove the two assignments to avoid overwriting the request when the function returns immediately.
138-145:task_instanceparameter lacks a type hint.All other parameters of
execute_jobare typed. As per coding guidelines, "Always add type hints to all function parameters and return values in Python code."Proposed fix
- task_instance, + task_instance: Any,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 138 - 145, The execute_job function signature is missing a type hint for the task_instance parameter; update the signature in execute_job(request_data: dict, project_id: int, organization_id: int, job_id: str, task_id: str, task_instance) -> dict to include an explicit type for task_instance (prefer the concrete type used elsewhere in the codebase, e.g., TaskInstance or Celery Task/TaskInstance if applicable); if the exact type is not available, import and use typing.Any (from typing import Any) and annotate task_instance: Any, keeping the return type as dict.
267-267: Remove unused variable assignmentuser_sent_config_provider.This variable is assigned but never referenced in the function.
Proposed fix
- user_sent_config_provider = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` at line 267, Remove the unused local variable assignment user_sent_config_provider (assigned to an empty string) from the function where it appears; delete the line initializing user_sent_config_provider, ensure no remaining references exist, and run linters/tests to confirm no regressions.backend/app/services/llm/guardrails.py (1)
138-149: Validators that aren'tdictinstances are silently dropped.The list comprehensions on lines 138–143 and 144–149 filter with
isinstance(validator, dict), which silently discards any non-dict entries returned by the external API. Consider logging a warning when entries are dropped so unexpected response shapes don't go unnoticed.Proposed approach
+ non_dict_validators = [v for v in validators if not isinstance(v, dict)] + if non_dict_validators: + logger.warning( + f"[list_validators_config] Dropped {len(non_dict_validators)} non-dict validator entries" + ) + input_guardrails = [ validator for validator in validators if isinstance(validator, dict) and str(validator.get("stage", "")).lower() == "input" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 138 - 149, The comprehensions that build input_guardrails and output_guardrails silently ignore any validators that are not dicts (validators variable), which can hide unexpected API responses; modify the logic that constructs input_guardrails and output_guardrails to detect non-dict entries in validators, log a warning via the module logger (or existing logger) including the offending item and its index, and still skip non-dict entries so behavior is unchanged; locate the comprehensions that create input_guardrails and output_guardrails and add a small loop or comprehension with conditional that records a warning when isinstance(validator, dict) is false, referencing the validators variable and the input_guardrails/output_guardrails names.backend/app/tests/services/llm/test_guardrails.py (1)
191-203: Consider adding a test forlist_validators_configwhen the external API call fails.The short-circuit test is valuable, but there's no test verifying the error/re-raise behavior when the HTTP call fails (e.g.,
raise_for_statusthrows). This is the path that can crashexecute_jobif not handled (see thejobs.pycomment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/services/llm/test_guardrails.py` around lines 191 - 203, Add a new unit test for list_validators_config that verifies it propagates or re-raises HTTP errors when the external API call fails: patch app.services.llm.guardrails.httpx.Client (same as test_list_validators_config_empty_short_circuits_without_http), configure the mocked client's get() response to raise on raise_for_status (or to return a response whose raise_for_status side effect raises an httpx.HTTPStatusError), call list_validators_config with a non-empty validator_configs so the function does not short-circuit, and assert that the HTTP error is raised and that the mocked client was called; reference list_validators_config, mock_client_cls, and raise_for_status in your test.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/llm/jobs.py (1)
415-422:⚠️ Potential issue | 🔴 CriticalBug:
error=request.query.inputpasses aQueryInputobject, not a string.On Line 417,
request.query.inputis aQueryInputPydantic model, not astr.APIResponse.failure_response(error=...)expectsstr | list, so this will produce a garbled error message (the object's repr). This is also inconsistent with the input guardrails path (Line 259), which correctly passessafe_input["data"]["safe_text"].Additionally, Line 415 still uses
== Trueinstead of a truthiness check.Proposed fix
- if safe_output["data"]["rephrase_needed"] == True: + if safe_output["data"]["rephrase_needed"]: callback_response = APIResponse.failure_response( - error=request.query.input, + error=safe_output["data"]["safe_text"], metadata=request.request_metadata, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 415 - 422, The failure branch uses request.query.input (a QueryInput model) and a strict equality check; update it to check truthiness (if safe_output["data"]["rephrase_needed"]:) and pass a string error like the already-used safe_input["data"]["safe_text"] to APIResponse.failure_response instead of the QueryInput object; ensure you still call handle_job_error(job_id, request.callback_url, callback_response) after constructing APIResponse.failure_response so the callback receives a proper string error message.
🧹 Nitpick comments (4)
backend/app/services/llm/jobs.py (2)
424-433: Same dead mutation pattern on the output path.Line 425 sets
response.response.output.content.value = safe_output["error"]right before returning a failure — the mutated response is never used.Suggested removal
else: - response.response.output.content.value = safe_output["error"] - callback_response = APIResponse.failure_response( error=safe_output["error"], metadata=request.request_metadata,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 424 - 433, Remove the unused mutation that sets response.response.output.content.value to safe_output["error"] since the mutated response is never used before returning failure; instead just construct the failure callback_response via APIResponse.failure_response (using safe_output["error"] and request.request_metadata) and call handle_job_error(job_id, request.callback_url, callback_response). Ensure no other code depends on response.response.output.content.value at this point (references: response.response.output.content.value, safe_output, APIResponse.failure_response, handle_job_error, job_id, request.callback_url, callback_response).
265-275: Mutatingrequest.query.input.content.valuewith an error message (Line 267) before returning failure is confusing.On validation failure, Line 267 sets
request.query.input.content.value = safe_input["error"], but the request object is not used after this point — the function immediately returns a failure response. This mutation has no practical effect and muddies the semantics of the request object.Suggested removal
else: - # Update the text value with error message - request.query.input.content.value = safe_input["error"] - callback_response = APIResponse.failure_response( error=safe_input["error"], metadata=request.request_metadata,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/jobs.py` around lines 265 - 275, Remove the unnecessary mutation of request.query.input.content.value when validation fails: do not set request.query.input.content.value = safe_input["error"] since the request object is not used afterward; instead, only construct the APIResponse.failure_response (using safe_input["error"] and request.request_metadata) and return handle_job_error(job_id, request.callback_url, callback_response). Update any tests that relied on the mutated request object if present and ensure the early return path in the surrounding function (where request, safe_input, callback_response, handle_job_error, and job_id are used) still behaves identically without that side-effect.backend/app/services/llm/guardrails.py (2)
156-162:list_validators_configlogsvalidator_config_ids(UUIDs) on failure — verify this is acceptable.Line 159 logs the full list of
validator_config_ids. While UUIDs are generally not PII, in some contexts they could be considered sensitive identifiers. Logging just the count would be safer. As per coding guidelines, log messages should usemask_string(sensitive_value)for sensitive values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 156 - 162, The logger in list_validators_config currently prints the full validator_config_ids list on failure; update the logger.error call in that exception block (the one in list_validators_config referencing validator_config_ids, organization_id, project_id, endpoint) to avoid exposing raw UUIDs by either logging only the count (e.g., validator_config_count=len(validator_config_ids)) or applying mask_string to each id before including them, and keep other context fields unchanged; ensure you import/retain mask_string usage if chosen and replace the validator_config_ids interpolation in the log message with the safer representation.
101-105: UnnecessaryContent-Typeheader on a GET request.The
Content-Type: application/jsonheader is meaningless for a GET request with no body. Consider extracting the shared auth headers into a helper to avoid this kind of drift.Suggested refactor: extract common headers
+def _guardrails_headers() -> dict[str, str]: + return { + "accept": "application/json", + "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}", + } + + def run_guardrails_validation(...): ... - headers = { - "accept": "application/json", - "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}", - "Content-Type": "application/json", - } + headers = {**_guardrails_headers(), "Content-Type": "application/json"} ... def list_validators_config(...): ... - headers = { - "accept": "application/json", - "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}", - "Content-Type": "application/json", - } + headers = _guardrails_headers()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 101 - 105, The headers dict assigned to headers in guardrails.py includes an unnecessary "Content-Type" for a GET request; remove "Content-Type": "application/json" from the GET call and extract the shared auth/accept pair into a helper (e.g., get_guardrails_headers() that returns {"accept": "application/json", "Authorization": f"Bearer {settings.KAAPI_GUARDRAILS_AUTH}"}), then replace the inline headers variable with a call to that helper; keep adding "Content-Type" only in places that send a JSON body (POST/PUT) so callers can extend the helper result when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/api_keys.py`:
- Around line 87-96: The function verify_api_key_route is missing a return type
hint; update its signature to include an explicit return type such as ->
APIResponse[APIKeyVerifyResponse] (or -> APIResponse if generics aren't used) so
the signature becomes def verify_api_key_route(current_user: AuthContextDep) ->
APIResponse[APIKeyVerifyResponse]: and ensure any necessary imports for
APIResponse/typing generics are present; this targets the verify_api_key_route
function and the APIKeyVerifyResponse/APIResponse symbols.
In `@backend/app/services/llm/jobs.py`:
- Around line 213-233: The current code merges config_blob.input_guardrails and
config_blob.output_guardrails into validator_configs and relies on
list_validators_config to re-split by stage, which can misclassify validators;
modify execute_job to preserve ConfigBlob intent by calling
list_validators_config separately for input and output (e.g., call
list_validators_config(organization_id, project_id,
validator_configs=config_blob.input_guardrails) to populate input_guardrails and
likewise for output_guardrails), and fall back to empty lists on exception while
keeping the original job_id logging; ensure you reference config_blob,
input_guardrails, output_guardrails, list_validators_config, and job_id when
making the changes.
---
Outside diff comments:
In `@backend/app/services/llm/jobs.py`:
- Around line 415-422: The failure branch uses request.query.input (a QueryInput
model) and a strict equality check; update it to check truthiness (if
safe_output["data"]["rephrase_needed"]:) and pass a string error like the
already-used safe_input["data"]["safe_text"] to APIResponse.failure_response
instead of the QueryInput object; ensure you still call handle_job_error(job_id,
request.callback_url, callback_response) after constructing
APIResponse.failure_response so the callback receives a proper string error
message.
---
Nitpick comments:
In `@backend/app/services/llm/guardrails.py`:
- Around line 156-162: The logger in list_validators_config currently prints the
full validator_config_ids list on failure; update the logger.error call in that
exception block (the one in list_validators_config referencing
validator_config_ids, organization_id, project_id, endpoint) to avoid exposing
raw UUIDs by either logging only the count (e.g.,
validator_config_count=len(validator_config_ids)) or applying mask_string to
each id before including them, and keep other context fields unchanged; ensure
you import/retain mask_string usage if chosen and replace the
validator_config_ids interpolation in the log message with the safer
representation.
- Around line 101-105: The headers dict assigned to headers in guardrails.py
includes an unnecessary "Content-Type" for a GET request; remove "Content-Type":
"application/json" from the GET call and extract the shared auth/accept pair
into a helper (e.g., get_guardrails_headers() that returns {"accept":
"application/json", "Authorization": f"Bearer
{settings.KAAPI_GUARDRAILS_AUTH}"}), then replace the inline headers variable
with a call to that helper; keep adding "Content-Type" only in places that send
a JSON body (POST/PUT) so callers can extend the helper result when needed.
In `@backend/app/services/llm/jobs.py`:
- Around line 424-433: Remove the unused mutation that sets
response.response.output.content.value to safe_output["error"] since the mutated
response is never used before returning failure; instead just construct the
failure callback_response via APIResponse.failure_response (using
safe_output["error"] and request.request_metadata) and call
handle_job_error(job_id, request.callback_url, callback_response). Ensure no
other code depends on response.response.output.content.value at this point
(references: response.response.output.content.value, safe_output,
APIResponse.failure_response, handle_job_error, job_id, request.callback_url,
callback_response).
- Around line 265-275: Remove the unnecessary mutation of
request.query.input.content.value when validation fails: do not set
request.query.input.content.value = safe_input["error"] since the request object
is not used afterward; instead, only construct the APIResponse.failure_response
(using safe_input["error"] and request.request_metadata) and return
handle_job_error(job_id, request.callback_url, callback_response). Update any
tests that relied on the mutated request object if present and ensure the early
return path in the surrounding function (where request, safe_input,
callback_response, handle_job_error, and job_id are used) still behaves
identically without that side-effect.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/app/services/llm/guardrails.py (2)
121-153: Nested function definitions insidetry/withblocks reduce readability.
_build_paramsand_fetch_by_idsare both defined inline within thetry/withscope. Extracting them as module-level or class-level helpers would improve readability and testability, while keeping the sameclientusage pattern (passclientas a parameter).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 121 - 153, Extract the nested helper functions _fetch_by_ids and _build_params out of the try/with block into module-level or class-level helpers and have them accept the httpx.Client instance (client) as an argument; preserve the current logic inside _fetch_by_ids (empty-list check, client.get(endpoint, params=_build_params(...), headers=headers), response.raise_for_status(), JSON payload type checks for dict, success flag, data list type check, and filtering to dicts) and keep the surrounding try/with usage in the original function to manage client lifetime, then call the new helpers (e.g., _fetch_by_ids(client, input_validator_config_ids) and _fetch_by_ids(client, output_validator_config_ids)) and return the same (input_guardrails, output_guardrails) tuple.
56-66: URL construction may double-slash ifKAAPI_GUARDRAILS_URLalready ends with/.Line 59 unconditionally appends
"/", while line 112 appends"/validators/configs/". IfKAAPI_GUARDRAILS_URLis set with a trailing slash, you gethttps://host/api//orhttps://host/api//validators/configs/. Consider normalizing once:base = settings.KAAPI_GUARDRAILS_URL.rstrip("/")and using
baseconsistently in both functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 56 - 66, Normalize the base KAAPI URL to avoid double slashes by trimming any trailing slash once and reusing it: create a local variable from settings.KAAPI_GUARDRAILS_URL (e.g., base = settings.KAAPI_GUARDRAILS_URL.rstrip("/")) and replace the two ad-hoc f-strings that build the endpoint (the client.post call that uses f"{settings.KAAPI_GUARDRAILS_URL}/" and the other call that uses f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/") to use f"{base}/" and f"{base}/validators/configs/" respectively so URLs never become "//".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/llm/guardrails.py`:
- Around line 112-113: Add a blank line between the top-level assignment to the
variable endpoint and the nested function definition _build_params to satisfy
Black's formatting rules; locate the endpoint =
f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/" assignment and insert a
single empty line immediately before the def _build_params(validator_ids:
list[UUID]) -> dict[str, Any]: declaration.
In `@backend/app/services/llm/jobs.py`:
- Around line 408-415: The conditional and error payload are wrong: change the
equality check to a truthiness check (use if
safe_output["data"]["rephrase_needed"]:), and pass the actual input string (not
the TextInput model) to APIResponse.failure_response — e.g., use
request.query.input.content.value (or the field that contains the raw text) as
the error argument; keep the subsequent call to handle_job_error(job_id,
request.callback_url, callback_response) unchanged. Ensure you reference
safe_output, APIResponse.failure_response, request.query.input, and
handle_job_error when making the change.
In `@backend/app/tests/services/llm/test_guardrails.py`:
- Around line 148-189: Run the code formatter (black) on this test file to fix
line-length formatting issues; if Black still reports over-length for the long
test function name test_list_validators_config_fetches_input_and_output_by_refs
or the assertions that access mock_client.get.call_args_list and
first_call_kwargs/second_call_kwargs, shorten the test name or split those long
assertion lines into multiple shorter statements (e.g., assign the ids list to a
local var before asserting) so all lines are within Black's max line length,
then re-run Black and CI.
---
Nitpick comments:
In `@backend/app/services/llm/guardrails.py`:
- Around line 121-153: Extract the nested helper functions _fetch_by_ids and
_build_params out of the try/with block into module-level or class-level helpers
and have them accept the httpx.Client instance (client) as an argument; preserve
the current logic inside _fetch_by_ids (empty-list check, client.get(endpoint,
params=_build_params(...), headers=headers), response.raise_for_status(), JSON
payload type checks for dict, success flag, data list type check, and filtering
to dicts) and keep the surrounding try/with usage in the original function to
manage client lifetime, then call the new helpers (e.g., _fetch_by_ids(client,
input_validator_config_ids) and _fetch_by_ids(client,
output_validator_config_ids)) and return the same (input_guardrails,
output_guardrails) tuple.
- Around line 56-66: Normalize the base KAAPI URL to avoid double slashes by
trimming any trailing slash once and reusing it: create a local variable from
settings.KAAPI_GUARDRAILS_URL (e.g., base =
settings.KAAPI_GUARDRAILS_URL.rstrip("/")) and replace the two ad-hoc f-strings
that build the endpoint (the client.post call that uses
f"{settings.KAAPI_GUARDRAILS_URL}/" and the other call that uses
f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/") to use f"{base}/" and
f"{base}/validators/configs/" respectively so URLs never become "//".
| endpoint = f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/" | ||
| def _build_params(validator_ids: list[UUID]) -> dict[str, Any]: |
There was a problem hiding this comment.
Missing blank line before nested function definition (Black violation).
The CI pipeline reports that Black formatting changed this file. The missing blank line between the endpoint assignment and the _build_params definition is the likely cause.
Proposed fix
endpoint = f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/"
+
def _build_params(validator_ids: list[UUID]) -> dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/llm/guardrails.py` around lines 112 - 113, Add a blank
line between the top-level assignment to the variable endpoint and the nested
function definition _build_params to satisfy Black's formatting rules; locate
the endpoint = f"{settings.KAAPI_GUARDRAILS_URL}/validators/configs/" assignment
and insert a single empty line immediately before the def
_build_params(validator_ids: list[UUID]) -> dict[str, Any]: declaration.
| if safe_output["data"]["rephrase_needed"] == True: | ||
| callback_response = APIResponse.failure_response( | ||
| error=request.query.input, | ||
| metadata=request.request_metadata, | ||
| ) | ||
| return handle_job_error( | ||
| job_id, request.callback_url, callback_response | ||
| ) |
There was a problem hiding this comment.
Bug: request.query.input is a TextInput object, not a string — passing it as error will produce a garbled error message.
APIResponse.failure_response(error=...) expects str | list, but request.query.input is a TextInput Pydantic model. This will call str() on the model, producing something like type='text' content=TextContent(value='...') in the error field sent to the callback.
Additionally, == True should be a truthiness check per Ruff E712 (flagged by static analysis and a previous review).
Proposed fix
- if safe_output["data"]["rephrase_needed"] == True:
+ if safe_output["data"]["rephrase_needed"]:
callback_response = APIResponse.failure_response(
- error=request.query.input,
+ error=safe_output["data"]["safe_text"],
metadata=request.request_metadata,
)🧰 Tools
🪛 Ruff (0.15.0)
[error] 408-408: Avoid equality comparisons to True; use safe_output["data"]["rephrase_needed"]: for truth checks
Replace with safe_output["data"]["rephrase_needed"]
(E712)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/services/llm/jobs.py` around lines 408 - 415, The conditional and
error payload are wrong: change the equality check to a truthiness check (use if
safe_output["data"]["rephrase_needed"]:), and pass the actual input string (not
the TextInput model) to APIResponse.failure_response — e.g., use
request.query.input.content.value (or the field that contains the raw text) as
the error argument; keep the subsequent call to handle_job_error(job_id,
request.callback_url, callback_response) unchanged. Ensure you reference
safe_output, APIResponse.failure_response, request.query.input, and
handle_job_error when making the change.
|
|
||
| @patch("app.services.llm.guardrails.httpx.Client") | ||
| def test_list_validators_config_fetches_input_and_output_by_refs(mock_client_cls) -> None: | ||
| input_validator_configs = [Validator(validator_config_id=uuid.uuid4())] | ||
| output_validator_configs = [Validator(validator_config_id=uuid.uuid4())] | ||
|
|
||
| input_response = MagicMock() | ||
| input_response.raise_for_status.return_value = None | ||
| input_response.json.return_value = { | ||
| "success": True, | ||
| "data": [{"type": "uli_slur_match", "config": {"severity": "high"}}], | ||
| } | ||
| output_response = MagicMock() | ||
| output_response.raise_for_status.return_value = None | ||
| output_response.json.return_value = { | ||
| "success": True, | ||
| "data": [{"type": "gender_assumption_bias"}], | ||
| } | ||
|
|
||
| mock_client = MagicMock() | ||
| mock_client.get.side_effect = [input_response, output_response] | ||
| mock_client_cls.return_value.__enter__.return_value = mock_client | ||
|
|
||
| input_guardrails, output_guardrails = list_validators_config( | ||
| input_validator_configs=input_validator_configs, | ||
| output_validator_configs=output_validator_configs, | ||
| organization_id=1, | ||
| project_id=1, | ||
| ) | ||
|
|
||
| assert input_guardrails == [{"type": "uli_slur_match", "config": {"severity": "high"}}] | ||
| assert output_guardrails == [{"type": "gender_assumption_bias"}] | ||
| assert mock_client.get.call_count == 2 | ||
|
|
||
| first_call_kwargs = mock_client.get.call_args_list[0].kwargs | ||
| second_call_kwargs = mock_client.get.call_args_list[1].kwargs | ||
| assert first_call_kwargs["params"]["ids"] == [ | ||
| str(v.validator_config_id) for v in input_validator_configs | ||
| ] | ||
| assert second_call_kwargs["params"]["ids"] == [ | ||
| str(v.validator_config_id) for v in output_validator_configs | ||
| ] |
There was a problem hiding this comment.
CI reports Black formatting failure — likely caused by the long function name at line 150.
The line def test_list_validators_config_fetches_input_and_output_by_refs(mock_client_cls) -> None: and assertion lines (178-179) likely exceed Black's line length. Run black to auto-format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/tests/services/llm/test_guardrails.py` around lines 148 - 189,
Run the code formatter (black) on this test file to fix line-length formatting
issues; if Black still reports over-length for the long test function name
test_list_validators_config_fetches_input_and_output_by_refs or the assertions
that access mock_client.get.call_args_list and
first_call_kwargs/second_call_kwargs, shorten the test name or split those long
assertion lines into multiple shorter statements (e.g., assign the ids list to a
local var before asserting) so all lines are within Black's max line length,
then re-run Black and CI.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/services/llm/guardrails.py (2)
106-110: Nit:Content-Typeheader is unnecessary for GET requests.This is a GET request with no body;
Content-Type: application/jsonhas no effect and can be removed to keep things tidy.acceptandAuthorizationare sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 106 - 110, Remove the unnecessary "Content-Type: application/json" header from the GET request in guardrails.py: locate the function that builds the headers dictionary for the GET call (the headers object that currently includes "accept", "Authorization", and "Content-Type") and delete the "Content-Type" entry so only "accept" and "Authorization" remain when performing the GET; leave all other logic and header keys untouched.
153-155: Silent filtering of non-dict entries may mask upstream API issues.Lines 153-155 silently discard any element in
datathat isn't adict. If the upstream API returns an unexpected shape for individual validators, this hides the problem. Consider logging a warning when items are dropped so unexpected responses are observable.Suggested improvement
- return [ - validator for validator in validators if isinstance(validator, dict) - ] + invalid = [v for v in validators if not isinstance(v, dict)] + if invalid: + logger.warning( + f"[list_validators_config] Dropped {len(invalid)} non-dict entries from validator response." + ) + return [v for v in validators if isinstance(v, dict)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/llm/guardrails.py` around lines 153 - 155, The code currently silently drops non-dict elements from the collection named "data" in backend/app/services/llm/guardrails.py; update the filtering loop that processes "data" (the block around lines 153-155) to log a warning for each dropped item instead of just discarding it: use the module logger (or create one via logging.getLogger(__name__)) and call logger.warning with the index, type(item) and a short repr of the item so upstream API shape issues are observable, while preserving the existing behavior of keeping only dicts for downstream processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/app/services/llm/guardrails.py`:
- Around line 56-63: Normalize the KAAPI_GUARDRAILS_URL value to avoid double
slashes: when reading the environment variable (KAAPI_GUARDRAILS_URL) or before
building URLs in guardrails.py, call .rstrip("/") on the base value and then
concatenate suffixes ("/" or "/validators/configs/") so you never produce "//".
Update the code paths that currently append "/" unconditionally (the places
constructing the base URL and the validators config URL) to use the normalized
base (e.g., base_url = os.getenv("KAAPI_GUARDRAILS_URL", "").rstrip("/")) and
then join suffixes, ensuring all references that build URLs in this module use
that normalized value.
---
Nitpick comments:
In `@backend/app/services/llm/guardrails.py`:
- Around line 106-110: Remove the unnecessary "Content-Type: application/json"
header from the GET request in guardrails.py: locate the function that builds
the headers dictionary for the GET call (the headers object that currently
includes "accept", "Authorization", and "Content-Type") and delete the
"Content-Type" entry so only "accept" and "Authorization" remain when performing
the GET; leave all other logic and header keys untouched.
- Around line 153-155: The code currently silently drops non-dict elements from
the collection named "data" in backend/app/services/llm/guardrails.py; update
the filtering loop that processes "data" (the block around lines 153-155) to log
a warning for each dropped item instead of just discarding it: use the module
logger (or create one via logging.getLogger(__name__)) and call logger.warning
with the index, type(item) and a short repr of the item so upstream API shape
issues are observable, while preserving the existing behavior of keeping only
dicts for downstream processing.
3 participants
Summary
Target issue is 49
We have to integrate config management changes between kaapi_guardrails and kaapi_backend. This will allow us to control which org / project uses which validators and run guardrails and perform other operations.
This PR is making the following changes -
input_guardrailsandoutput_guardrailsparameter in ConfigBlob which contains the list of input and output validators with their validator_config_id.validator_config_ids.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Summary by CodeRabbit
Refactor
New Features
Tests
Documentation