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:
📝 WalkthroughWalkthroughThis pull request introduces a ban list management system to the guardrails API, adds multitenant authentication via API keys with external backend integration, extends database models for request and validator logs with organization/project tracking, and establishes comprehensive test infrastructure with seeded data. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as FastAPI Route
participant Validator as validate_multitenant_key
participant Fetcher as _fetch_tenant_from_backend
participant HTTP as httpx.get
participant KAAPI as KAAPI Backend
participant Ctx as TenantContext
Client->>API: Request with X-API-KEY header
API->>Validator: Depends(validate_multitenant_key)
Validator->>Validator: Extract X-API-KEY from header
Validator->>Fetcher: Call with token
Fetcher->>Fetcher: Build auth URL + headers
Fetcher->>HTTP: GET KAAPI_AUTH_URL<br/>with X-API-KEY header (5s timeout)
HTTP->>KAAPI: HTTP GET request
KAAPI-->>HTTP: 200 + JSON<br/>{organization_id, project_id}
HTTP-->>Fetcher: Response
Fetcher->>Fetcher: Parse JSON & validate
Fetcher->>Ctx: Create TenantContext
Ctx-->>Fetcher: Instance
Fetcher-->>Validator: Return TenantContext
Validator-->>API: TenantContext resolved
API->>API: Execute endpoint with auth context
API-->>Client: Response with data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@backend/app/alembic/versions/004_added_banlist_config.py`:
- Around line 21-41: The migration creates the ban_list table without a
uniqueness constraint, so the IntegrityError handling in BanListCrud.create and
BanListCrud.update is dead code; either add a unique constraint to enforce
unique (name, organization_id, project_id) or remove that exception handling. To
fix by enforcing uniqueness, update the upgrade() in the
004_added_banlist_config.py migration to add a UniqueConstraint on ('name',
'organization_id', 'project_id') (e.g., include
sa.UniqueConstraint('name','organization_id','project_id') in op.create_table or
call op.create_unique_constraint after table creation) and add the corresponding
drop (op.drop_constraint) in downgrade(); reference the ban_list table and
BanListCrud.create / BanListCrud.update so the CRUD IntegrityError handling
becomes meaningful.
In `@backend/app/api/routes/banlist_configs.py`:
- Around line 64-78: The update_banlist and delete_banlist routes allow mutating
public ban lists because banlist_crud.get() skips ownership for obj.is_public
and banlist_crud.update/delete do not verify owner; add an explicit ownership
check in both route handlers after fetching obj (in update_banlist and
delete_banlist) that compares the object's owner (obj.organization_id or
obj.owner_id) against the caller's organization (use the AuthDep value currently
ignored in the signature or the organization_id parameter) and raise an
authorization error (HTTP 403) if they differ; ensure the check runs for
mutating operations on public lists so only the owning organization can update
or delete, then proceed to call banlist_crud.update or banlist_crud.delete when
the ownership check passes.
- Around line 24-32: create_banlist currently calls banlist_crud.create without
verifying the caller's tenant membership; before invoking banlist_crud.create,
extract the authenticated user's identity from the AuthDep (or accept/extend
AuthDep to return a user object), then verify that user belongs to the provided
organization_id and project_id (either by calling the existing check_owner()
helper in the CRUD layer or by querying the membership via the SessionDep), and
only proceed to call banlist_crud.create if that validation succeeds; if
validation fails, return an appropriate authorization error instead of creating
the banlist.
In `@backend/app/crud/banlist.py`:
- Around line 14-38: The create method for BanList currently only catches
IntegrityError, so it must mirror the update/delete behavior: wrap
session.commit() in a try block that catches IntegrityError (raising the 400
HTTPException) and also catches a generic Exception to session.rollback() and
re-raise as an HTTPException(500) (or propagate after rollback) to ensure the DB
session is not left broken; update the create function (BanList.create, where
session.commit() is called after session.add(obj) and before
session.refresh(obj)) to perform the same rollback-on-any-exception logic used
by update and delete.
In `@backend/app/models/config/banlist.py`:
- Around line 26-29: The BanList model's description field is declared
Optional[str] but marked nullable=False, causing a mismatch; change the type
annotation on the description field from Optional[str] to str (i.e., make it
required) so it aligns with sa_column nullable=False and the ORM won't try to
insert NULL, or alternatively set sa_column_kwargs nullable=True if you want to
allow nulls—update the declaration for the description Field in the BanList
model accordingly (adjust the type annotation and/or the sa_column_kwargs
nullable flag).
- Around line 53-58: The Field declaration for domain is inconsistent: it's
typed as str but given default=None while sa_column_kwargs sets nullable=False;
fix by making domain required—remove default=None from the Field for the domain
attribute in banlist.py (keep type str and nullable=False) so the
Pydantic/SQLAlchemy model enforces a value on creation; update any code that
instantiates the model to provide domain where missing.
In `@backend/app/schemas/banlist.py`:
- Around line 8-9: BanListBase currently sets model_config =
ConfigDict(extra="allow"), which lets BanListCreate accept arbitrary request
fields that then flow into the CRUD create path (see
BanList(**data.model_dump(), ...) in the create method); change BanListBase to
forbid extra fields (use ConfigDict(extra="forbid") or remove the model_config
so default behavior applies) so unexpected fields are rejected at validation
time and cannot reach the create logic.
🧹 Nitpick comments (13)
backend/pyproject.toml (1)
25-25: Consider adding an upper-bound pin forguardrails-ai.Most dependencies in this file use an upper bound (e.g.,
<X.0.0), butguardrails-ai[hub]>=0.8.0is open-ended. A major version bump could introduce breaking changes. Consider constraining it, e.g.,guardrails-ai[hub]>=0.8.0,<1.0.0.backend/app/tests/test_banlist_configs.py (2)
113-125: Assert thatcrud.deletewas actually called.
test_delete_successonly checksresult.successbut never verifies thatcrud.deletewas invoked. Without this, the test would pass even if the delete call were accidentally removed from the route handler.Proposed fix
result = delete_banlist( id=TEST_ID, organization_id=TEST_ORGANIZATION_ID, project_id=TEST_PROJECT_ID, session=mock_session, _=None, ) assert result.success is True + crud.delete.assert_called_once_with(mock_session, sample_banlist)
53-65: Consider assertingcrud.createwas called with the expected arguments.Same pattern applies to the other tests — verifying that the CRUD method was called with the correct arguments strengthens confidence that the route is wiring parameters correctly, not just returning a canned mock.
backend/app/schemas/banlist.py (1)
1-3: Remove unused imports:datetimeandList.Neither
datetimenorListare used in this file. Ruff also flagsList(UP035).Proposed fix
from uuid import UUID -from datetime import datetime -from typing import List, Optional +from typing import Optionalbackend/app/tests/test_banlists_integration.py (2)
65-69: Method namelistshadows the Python built-in.Consider renaming to
list_banlistsorlist_allto avoid shadowingbuiltins.list. It won't break anything here, but it can cause subtle issues if someone later useslist(...)inside this class.
81-98: Consider adding a test for duplicate creation (IntegrityError path).The CRUD layer handles
IntegrityErrorand returns a 400. This error path isn't exercised by any integration test. A test creating the same ban list twice would cover it.backend/app/alembic/versions/004_added_banlist_config.py (2)
31-32:created_atandupdated_atlackserver_default.If rows are ever inserted outside the ORM (manual SQL, other services, data migrations), these columns will fail the
NOT NULLconstraint. Consider addingserver_default=sa.func.now()for resilience.
42-43: Downgrade should drop indexes explicitly before dropping the table.While PostgreSQL automatically drops indexes when the table is dropped, being explicit in the downgrade makes the migration symmetric and portable across databases.
backend/app/models/config/banlist.py (1)
1-3: Remove unused importList.
Listfromtypingis imported but not used; the code uses the built-inlist[str]syntax throughout. Ruff also flags this (UP035).Proposed fix
from datetime import datetime -from typing import List, Optional +from typing import Optional from uuid import UUID, uuid4backend/app/crud/banlist.py (3)
57-75: No pagination on thelistquery — may return unbounded results.The
listmethod returns all matching records (own + all public from every org). As the number of public ban lists grows, this could become a performance issue. Consider addinglimit/offset(or cursor-based) pagination.
2-2: Use built-inlistinstead oftyping.List.
typing.Listis deprecated since Python 3.9. The built-inlistis already used elsewhere in this codebase (e.g.,list[str]in the model). This aligns with the Ruff UP035 hint.Proposed fix
-from typing import List, Optional +from typing import OptionalAnd on line 63:
- ) -> List[BanList]: + ) -> list[BanList]:
118-118: Add trailing newline at end of file.Per Ruff W292 and POSIX convention.
backend/app/api/routes/banlist_configs.py (1)
34-46: List endpoint looks correct, but inherits the pagination concern from the CRUD layer.As noted in the CRUD review, the
listmethod returns unbounded results. Consider addinglimit/offsetquery params here to support pagination.
Collaborator
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/api/deps.py`:
- Line 70: Add a trailing newline at the end of the file so the file ends with a
newline character; specifically update the file containing the TenantDep
definition (Annotated[Tenant, Depends(get_tenant)]) to ensure there is a newline
after the final line to satisfy W292.
In `@backend/app/tests/test_banlists_integration.py`:
- Line 14: The tests currently build a DEFAULT_QUERY string and append
?organization_id=...&project_id=... to helper request URLs, but the application
now uses TenantDep headers (X-Org-Id / X-Project-Id) so those query params are
dead; remove DEFAULT_QUERY and any usage of the query string in helper functions
and test calls in test_banlists_integration.py (and the helper methods
referenced there) and ensure requests rely solely on the TenantDep headers
already being set (X-Org-Id, X-Project-Id) to derive tenant context.
- Around line 242-251: The test test_delete_wrong_owner currently sends a stale
query string in the DELETE call
(f"{BASE_URL}{ban_id}/?organization_id=999&project_id=999") which is ignored;
remove the query params and call integration_client.delete with the resource
path only (e.g. f"{BASE_URL}{ban_id}/") and continue to pass the owner headers
via self._headers(999, 999) so the request relies on headers for auth/ownership.
🧹 Nitpick comments (1)
backend/app/api/deps.py (1)
48-67: Falsy check onintheaders will reject valid0IDs and is partially redundant.
Header(...)already makes the headers required — FastAPI returns 422 if they're missing or non-integer. Theif not x_org_idcheck on line 58 only additionally rejects0. If org/project IDs are always positive, consider making the intent explicit with<= 0; if0could be valid, this is a bug.Suggested clarification
- if not x_org_id or not x_project_id: + if x_org_id <= 0 or x_project_id <= 0: raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="Missing tenant headers", )
AkhileshNegi
requested changes
Feb 10, 2026
nishika26
previously requested changes
Feb 10, 2026
rkritika1508
force-pushed
the
feat/banlist-management
branch
from
0adff7e to
8c0b7de
Compare
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/tests/test_validator_configs_integration.py (1)
59-61:⚠️ Potential issue | 🟠 MajorInconsistent URL construction —
update_validatoris missing the trailing slash.
get_validator(line 47) anddelete_validator(line 65) use{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}(with trailing/), butupdate_validatoruses{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}(no trailing/). This produces a different path that may trigger a 307 redirect in FastAPI, potentially causing the PATCH body to be dropped.Fix: add trailing slash
def update_validator(self, client, validator_id, payload): """Helper to update a validator.""" - return client.patch(f"{BASE_URL}{validator_id}{DEFAULT_QUERY_PARAMS}", json=payload) + return client.patch(f"{BASE_URL}{validator_id}/{DEFAULT_QUERY_PARAMS}", json=payload)
🤖 Fix all issues with AI agents
In `@backend/app/tests/test_banlists_integration.py`:
- Line 5: The clear_database fixture is importing the production engine (engine)
instead of using the test_engine provided by conftest.py; update clear_database
to use the conftest test_engine (or move the clear_database fixture into
conftest alongside other DB fixtures) so it runs against the test database
instance; locate the clear_database fixture in test_banlists_integration.py (and
similarly in test_validator_configs_integration.py) and replace the engine
reference with test_engine (or relocate the entire fixture into conftest.py) to
ensure the fixture uses the same test_engine configuration as other DB fixtures.
In `@backend/app/tests/test_validator_configs_integration.py`:
- Around line 5-6: The clear_database fixture is importing and using the
production engine from app.core.db instead of the test_engine set up in
conftest; update the import in test_validator_configs_integration.py to use
test_engine (the same symbol used in conftest) and ensure any references inside
the clear_database fixture call/operate on test_engine rather than engine so the
fixture clears the test database during integration tests.
🧹 Nitpick comments (2)
backend/app/tests/conftest.py (1)
86-93: Redundant deletes inseed_db—clean_db(autouse) already clears all tables.Since
clean_dbisautouse=Trueand runs before every test function (clearing all tables), the explicitdelete(BanList)/delete(ValidatorConfig)inseed_dbare redundant. You can simplify to just seed and yield.Simplified seed_db
`@pytest.fixture`(scope="function") def seed_db(): with Session(test_engine) as session: - session.exec(delete(BanList)) - session.exec(delete(ValidatorConfig)) - session.commit() seed_test_data(session) yieldbackend/app/tests/test_validator_configs_integration.py (1)
49-57:list_validatorsrebuildsDEFAULT_QUERY_PARAMSinline instead of reusing the constant.Lines 51-54 duplicate the same string that
DEFAULT_QUERY_PARAMS(line 17-20) already defines.DRY fix
def list_validators(self, client, **query_params): """Helper to list validators with optional filters.""" - params_str = ( - f"?organization_id={VALIDATOR_INTEGRATION_ORGANIZATION_ID}" - f"&project_id={VALIDATOR_INTEGRATION_PROJECT_ID}" - ) + params_str = DEFAULT_QUERY_PARAMS if query_params: params_str += "&" + "&".join(f"{k}={v}" for k, v in query_params.items()) return client.get(f"{BASE_URL}{params_str}")
AkhileshNegi
requested changes
Feb 11, 2026
rkritika1508
force-pushed
the
feat/banlist-management
branch
from
fe79818 to
d367855
Compare
rkritika1508
force-pushed
the
feat/banlist-management
branch
from
d367855 to
3013e48
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/pyproject.toml (1)
44-44:⚠️ Potential issue | 🟡 Minor
pytest-asynciois unpinned while all other dev dependencies specify version constraints.All other entries in the
devgroup have both upper and lower bounds (e.g.,pytest<8.0.0,>=7.4.3), butpytest-asynciohas no version constraint, which risks pulling in breaking changes from future releases.Suggested fix
- "pytest-asyncio", + "pytest-asyncio<2.0.0,>=1.0.0",Adjust the range to match the version you've tested against. As of February 2026, pytest-asyncio is at v1.3.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/pyproject.toml` at line 44, The dev dependency entry for pytest-asyncio is unpinned; update the pytest-asyncio entry in the pyproject.toml dev dependencies to include an explicit version range (e.g., pin to a tested range like "pytest-asyncio>=1.2.0,<1.4.0" or "pytest-asyncio==1.3.0") so it matches the pattern used by other dev packages and prevents accidental breaking upgrades.backend/app/api/routes/guardrails.py (1)
40-52:⚠️ Potential issue | 🟡 Minor
_resolve_ban_list_banned_wordsexceptions leave the request log in a dangling state.If
_resolve_ban_list_banned_words(line 45) raises anHTTPException(404/403 from CRUD), the exception propagates directly without updating the request log's status toERROR. The log created on line 41 will remain without aresponse_textor final status.Consider wrapping the resolution + validation in a single flow that always finalizes the request log, similar to how
_validate_with_guarduses_finalize.🛡️ Proposed fix
try: request_log = request_log_crud.create(payload) except ValueError: return APIResponse.failure_response(error="Invalid request_id") - _resolve_ban_list_banned_words(payload, session) - return _validate_with_guard( - payload, - request_log_crud, - request_log.id, - validator_log_crud, - suppress_pass_logs, - ) + try: + _resolve_ban_list_banned_words(payload, session) + except HTTPException as exc: + request_log_crud.update( + request_log_id=request_log.id, + request_status=RequestStatus.ERROR, + request_log_update=RequestLogUpdate( + response_text=exc.detail, + response_id=uuid.uuid4(), + ), + ) + raise + + return _validate_with_guard( + payload, + request_log_crud, + request_log.id, + validator_log_crud, + suppress_pass_logs, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/guardrails.py` around lines 40 - 52, The request log created by request_log_crud.create is left incomplete if _resolve_ban_list_banned_words raises an HTTPException; wrap the call (and subsequent _validate_with_guard invocation) in a try/except/finally flow that ensures the request log is finalized on any exception—use the same pattern as _validate_with_guard/_finalize: call _resolve_ban_list_banned_words(payload, session) inside try, run _validate_with_guard on success, and in except (catch HTTPException and generic Exception) call request_log_crud.update or _finalize with request_log.id and an ERROR status and appropriate response_text before re-raising or returning an APIResponse.failure_response; reference request_log_crud.create, request_log.id, _resolve_ban_list_banned_words, _validate_with_guard, _finalize, validator_log_crud, and suppress_pass_logs to locate and implement the change.
🧹 Nitpick comments (5)
backend/app/api/routes/ban_list_configs.py (1)
23-31: Consider splittingHTTPExceptioninto its ownexceptclause for clarity.The
isinstancecheck inside a bareexcept Exceptionworks but is non-idiomatic. A dedicatedexcept HTTPExceptionclause makes the intent explicit and avoids accidentally processingHTTPExceptionthrough_safe_error_messageif theisinstancecheck is ever accidentally removed. This pattern repeats in all five endpoints.♻️ Proposed fix (apply to all endpoints)
try: response_model = ban_list_crud.create( session, payload, organization_id, project_id ) return APIResponse.success_response(data=response_model) + except HTTPException: + raise except Exception as exc: - if isinstance(exc, HTTPException): - raise exc return APIResponse.failure_response(error=_safe_error_message(exc))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_list_configs.py` around lines 23 - 31, The try/except block around ban_list_crud.create currently catches Exception and then checks isinstance(exc, HTTPException); instead refactor to have a separate except HTTPException: clause that simply re-raises the exception, followed by a generic except Exception as exc: that returns APIResponse.failure_response(error=_safe_error_message(exc)). Apply this same pattern to the other four endpoints so HTTPException is never handled by the generic error path; locate the blocks by searching for ban_list_crud.create, APIResponse.failure_response, and _safe_error_message to update them.backend/app/core/validators/config/ban_list_safety_validator_config.py (1)
1-1: Replace deprecatedtyping.Listwith built-inlist.Ruff UP035 flags
typing.Listas deprecated. Uselist[str]directly on line 12.♻️ Proposed fix
-from typing import List, Literal, Optional +from typing import Literal, OptionalAnd on line 12:
- banned_words: Optional[List[str]] = None # list of banned words to be redacted + banned_words: Optional[list[str]] = None # list of banned words to be redacted🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/validators/config/ban_list_safety_validator_config.py` at line 1, Replace the deprecated typing.List usage in the module by removing List from the import and switching any annotations like List[str] to the built-in generic form list[str]; e.g., update the import line to keep only Literal and Optional (from typing import Literal, Optional) and change the annotation (e.g., bad_words: List[str]) to bad_words: list[str] inside the BanListSafetyValidatorConfig (or any variables/functions using List) so the code uses list[str] instead of typing.List.backend/app/api/routes/guardrails.py (1)
85-99: Redundantvalidator.type != BAN_LISTcheck.Since
isinstance(validator, BanListSafetyValidatorConfig)on line 87 already guaranteestype == "ban_list"(it's aLiteral["ban_list"]), thevalidator.type != BAN_LISTguard on line 90 is redundant.♻️ Proposed simplification
- if validator.type != BAN_LIST or validator.banned_words is not None: + if validator.banned_words is not None: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/guardrails.py` around lines 85 - 99, In _resolve_ban_list_banned_words, remove the redundant runtime check for validator.type != BAN_LIST because isinstance(validator, BanListSafetyValidatorConfig) already implies type == "ban_list"; update the loop so it only continues if not isinstance(validator, BanListSafetyValidatorConfig) or if validator.banned_words is not None, then load the ban list via ban_list_crud.get and assign to validator.banned_words as before.backend/app/tests/test_banlists_integration.py (2)
88-94:test_list_successasserts a hardcoded count of 4 — will break if seed data changes.Consider asserting
len(data) > 0or deriving the expected count from seed data constants to make the test less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 88 - 94, The test TestListBanLists.test_list_success currently asserts a hardcoded length of 4, making it brittle if seed data changes; update the assertion in test_list_success to either assert len(data) > 0 or compute the expected count from the seed data constants used by the seed_db fixture (or a shared SEED_BANLIST_COUNT constant) and compare against that value; locate the check in the list(...) response handling inside test_list_success and replace the hardcoded 4 with the non-brittle assertion referencing the seed constant or a >0 check.
40-44:updateanddeletehelpers don't acceptorg/projectoverrides, forcing raw URL construction in ownership tests.The
gethelper (line 31) acceptsorgandprojectparams, butupdateanddeletedon't. This forces tests liketest_update_public_wrong_owner_fails(line 192) andtest_delete_wrong_owner(line 230) to manually build URLs, which is inconsistent and error-prone.♻️ Proposed fix
- def update(self, client, id, payload): - return client.patch(f"{BASE_URL}{id}/{DEFAULT_QUERY}", json=payload) + def update(self, client, id, payload, org=BAN_LIST_INTEGRATION_ORGANIZATION_ID, project=BAN_LIST_INTEGRATION_PROJECT_ID): + return client.patch( + f"{BASE_URL}{id}/?organization_id={org}&project_id={project}", + json=payload, + ) - def delete(self, client, id): - return client.delete(f"{BASE_URL}{id}/{DEFAULT_QUERY}") + def delete(self, client, id, org=BAN_LIST_INTEGRATION_ORGANIZATION_ID, project=BAN_LIST_INTEGRATION_PROJECT_ID): + return client.delete( + f"{BASE_URL}{id}/?organization_id={org}&project_id={project}", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 40 - 44, The update and delete helpers must accept optional org and project overrides like the get helper does; change signatures of update(self, client, id, payload) and delete(self, client, id) to include org=None, project=None and construct the request URL using the same query-building logic used by get (reuse the same org/project handling and DEFAULT_QUERY/BASE_URL pattern) so tests can pass org/project without manually composing raw URLs; keep the endpoint format consistent with get (f"{BASE_URL}{id}/" + computed_query).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/app/api/routes/guardrails.py`:
- Around line 40-52: The request log created by request_log_crud.create is left
incomplete if _resolve_ban_list_banned_words raises an HTTPException; wrap the
call (and subsequent _validate_with_guard invocation) in a try/except/finally
flow that ensures the request log is finalized on any exception—use the same
pattern as _validate_with_guard/_finalize: call
_resolve_ban_list_banned_words(payload, session) inside try, run
_validate_with_guard on success, and in except (catch HTTPException and generic
Exception) call request_log_crud.update or _finalize with request_log.id and an
ERROR status and appropriate response_text before re-raising or returning an
APIResponse.failure_response; reference request_log_crud.create, request_log.id,
_resolve_ban_list_banned_words, _validate_with_guard, _finalize,
validator_log_crud, and suppress_pass_logs to locate and implement the change.
In `@backend/pyproject.toml`:
- Line 44: The dev dependency entry for pytest-asyncio is unpinned; update the
pytest-asyncio entry in the pyproject.toml dev dependencies to include an
explicit version range (e.g., pin to a tested range like
"pytest-asyncio>=1.2.0,<1.4.0" or "pytest-asyncio==1.3.0") so it matches the
pattern used by other dev packages and prevents accidental breaking upgrades.
---
Duplicate comments:
In `@backend/app/api/routes/ban_list_configs.py`:
- Line 12: The router prefix currently uses an underscore in the APIRouter
declaration (router = APIRouter(prefix="/guardrails/ban_lists", tags=["Ban
Lists"])) but the API spec and PR description require hyphens; update the prefix
string on the APIRouter call to use "/guardrails/ban-lists" so the router
variable (router) and APIRouter invocation align with the API convention.
In `@backend/app/core/validators/config/ban_list_safety_validator_config.py`:
- Around line 21-25: The build() method currently returns a no-op BanList when
self.banned_words is falsy; change build() to fail fast: if self.banned_words is
None and self.ban_list_id is set (indicating _resolve_ban_list_banned_words
should have run), raise a clear exception (or call resolve_on_fail()) instead of
passing an empty list to BanList; update the logic in build() to validate that
banned_words is a real list before constructing BanList and reference BanList,
build(), banned_words, ban_list_id, and _resolve_ban_list_banned_words so the
guard triggers when resolution hasn't occurred.
---
Nitpick comments:
In `@backend/app/api/routes/ban_list_configs.py`:
- Around line 23-31: The try/except block around ban_list_crud.create currently
catches Exception and then checks isinstance(exc, HTTPException); instead
refactor to have a separate except HTTPException: clause that simply re-raises
the exception, followed by a generic except Exception as exc: that returns
APIResponse.failure_response(error=_safe_error_message(exc)). Apply this same
pattern to the other four endpoints so HTTPException is never handled by the
generic error path; locate the blocks by searching for ban_list_crud.create,
APIResponse.failure_response, and _safe_error_message to update them.
In `@backend/app/api/routes/guardrails.py`:
- Around line 85-99: In _resolve_ban_list_banned_words, remove the redundant
runtime check for validator.type != BAN_LIST because isinstance(validator,
BanListSafetyValidatorConfig) already implies type == "ban_list"; update the
loop so it only continues if not isinstance(validator,
BanListSafetyValidatorConfig) or if validator.banned_words is not None, then
load the ban list via ban_list_crud.get and assign to validator.banned_words as
before.
In `@backend/app/core/validators/config/ban_list_safety_validator_config.py`:
- Line 1: Replace the deprecated typing.List usage in the module by removing
List from the import and switching any annotations like List[str] to the
built-in generic form list[str]; e.g., update the import line to keep only
Literal and Optional (from typing import Literal, Optional) and change the
annotation (e.g., bad_words: List[str]) to bad_words: list[str] inside the
BanListSafetyValidatorConfig (or any variables/functions using List) so the code
uses list[str] instead of typing.List.
In `@backend/app/tests/test_banlists_integration.py`:
- Around line 88-94: The test TestListBanLists.test_list_success currently
asserts a hardcoded length of 4, making it brittle if seed data changes; update
the assertion in test_list_success to either assert len(data) > 0 or compute the
expected count from the seed data constants used by the seed_db fixture (or a
shared SEED_BANLIST_COUNT constant) and compare against that value; locate the
check in the list(...) response handling inside test_list_success and replace
the hardcoded 4 with the non-brittle assertion referencing the seed constant or
a >0 check.
- Around line 40-44: The update and delete helpers must accept optional org and
project overrides like the get helper does; change signatures of update(self,
client, id, payload) and delete(self, client, id) to include org=None,
project=None and construct the request URL using the same query-building logic
used by get (reuse the same org/project handling and DEFAULT_QUERY/BASE_URL
pattern) so tests can pass org/project without manually composing raw URLs; keep
the endpoint format consistent with get (f"{BASE_URL}{id}/" + computed_query).
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (10)
backend/app/tests/test_banlist_configs.py (2)
106-124: No tests for error paths (exception handling in route handlers).The route handlers catch
Exceptionand re-raiseHTTPExceptionwhile returningAPIResponse.failure_responsefor other errors. Consider adding at least one test verifying that anHTTPExceptionfrom the CRUD layer propagates correctly (i.e., is re-raised, not swallowed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlist_configs.py` around lines 106 - 124, Add a test that ensures an HTTPException raised by the CRUD layer is propagated by the route handler: patch ban_list_crud.get to raise fastapi.HTTPException, call delete_ban_list (the route handler) and assert it raises HTTPException (use pytest.raises), and optionally assert ban_list_crud.delete was not called; this verifies delete_ban_list does not swallow HTTPException and re-raises it instead of returning APIResponse.failure_response.
48-59: Tests for create/list/get only verify the return value, not the CRUD call arguments.
test_update_successandtest_delete_successproperly assert that the CRUD layer is called with the correctorganization_id,project_id, andid. The create, list, and get tests skip this, so a bug that swaps or drops tenant parameters would go undetected. Consider addingassert_called_once_with(or inspectingcall_args) for consistency.💡 Example for create
result = create_ban_list( payload=create_payload, session=mock_session, auth=auth_context, ) + crud.create.assert_called_once_with( + mock_session, + create_payload, + BAN_LIST_TEST_ORGANIZATION_ID, + BAN_LIST_TEST_PROJECT_ID, + ) assert result.data == sample_ban_list🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlist_configs.py` around lines 48 - 59, The test test_create_calls_crud currently only asserts the return value; update it to also assert that ban_list_crud.create was invoked with the correct tenant args and payload — e.g. use crud.create.assert_called_once_with(...) (or inspect crud.create.call_args) to verify organization_id and project_id are passed from auth_context, the session is mock_session, and the payload is create_payload when calling create_ban_list; locate this in the test function test_create_calls_crud and add the single assert_called_once_with (or equivalent) check referencing crud.create, create_ban_list, mock_session, create_payload, and auth_context to match the pattern used in test_update_success/test_delete_success.backend/app/tests/conftest.py (1)
91-107: Test override strips theApiKeyprefix locally, but production sends the raw header value to the backend.In
deps.py,validate_multitenant_keypasses the raw (stripped) header value to_fetch_tenant_from_backend, which then forwards it as"ApiKey {token}"to the backend. The test override here does its own prefix parsing (lines 96–97), meaning the test "token" semantics diverge from production. This could mask integration issues if a test header like"ApiKey org999_project999"would behave differently in production vs tests.This is fine for now since the override is intentionally simplified, but worth documenting with a brief comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/conftest.py` around lines 91 - 107, The test override override_multitenant_key currently strips an "ApiKey " prefix itself, which diverges from production behavior in validate_multitenant_key and _fetch_tenant_from_backend that forward the raw header as "ApiKey {token}"; update the override_multitenant_key function to include a brief comment explaining this intentional simplification and the semantic difference from validate_multitenant_key/_fetch_tenant_from_backend so future readers know the test parses the header locally and may not mirror production header forwarding (keep the override logic unchanged, just add the explanatory comment near where app.dependency_overrides[validate_multitenant_key] is set).backend/app/api/deps.py (2)
80-122: Synchronoushttpx.geton every request — consider caching for high-throughput scenarios.
_fetch_tenant_from_backendmakes a blocking HTTP call to the backend on every API request. While this is correct for sync route handlers (FastAPI runs them in a threadpool), it adds latency and creates a hard dependency on the backend service's availability for every ban-list operation. If request volume grows, consider adding a short-lived TTL cache (e.g.,cachetools.TTLCachekeyed by token) to reduce redundant lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 80 - 122, _fetch_tenant_from_backend currently issues a synchronous httpx.get per request (using settings.KAAPI_BACKEND_CREDENTIAL_URL) which can add latency and backend dependency; add a short-lived TTL cache (e.g., cachetools.TTLCache keyed by the token) inside the module so the function first checks the cache for a TenantContext before calling httpx.get, populate the cache with the constructed TenantContext on successful responses, and keep existing error handling (HTTPException/_unauthorized, parsing, and type checks) intact; ensure cache timeout is configurable and use token as the cache key to avoid leaking contexts between tokens.
93-97: Useraise ... from Noneto suppress exception chaining in error handlers.Inside
exceptblocks, bareraise HTTPException(...)implicitly chains the caught exception. This can leak internal details (e.g., connection strings, hostnames) in tracebacks/logs. The same pattern applies at lines 104–105 and 116–122.♻️ Proposed fix
except httpx.RequestError: - raise HTTPException( + raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Auth service unavailable", - ) + ) from Noneexcept ValueError: - raise _unauthorized("Invalid API key") + raise _unauthorized("Invalid API key") from Noneexcept (KeyError, TypeError, ValueError): - raise _unauthorized("Invalid API key") + raise _unauthorized("Invalid API key") from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 93 - 97, The except blocks currently re-raise HTTPException using plain "raise HTTPException(...)" which causes implicit exception chaining and can leak internal details; update each handler that catches httpx.RequestError (the shown except httpx.RequestError and the similar handlers later) to re-raise the HTTPException with "from None" (i.e., raise HTTPException(... ) from None) so the original exception is suppressed in tracebacks/logs while preserving the intended HTTP 503 behavior.backend/app/api/routes/ban_list_configs.py (1)
21-29: Consider extracting the repetitive try/except/re-raise pattern.All five endpoints share identical exception handling boilerplate. A small helper or decorator would reduce duplication and ensure consistency if the error strategy changes.
💡 Example helper
from functools import wraps def handle_route_errors(func): `@wraps`(func) def wrapper(*args, **kwargs): try: return func(*args, **kwargs) except HTTPException: raise except Exception as exc: return APIResponse.failure_response(error=_safe_error_message(exc)) return wrapperThen each endpoint becomes:
`@router.post`("/", response_model=APIResponse[BanListResponse]) `@handle_route_errors` def create_ban_list(payload: BanListCreate, session: SessionDep, auth: MultitenantAuthDep): response_model = ban_list_crud.create(session, payload, auth.organization_id, auth.project_id) return APIResponse.success_response(data=response_model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_list_configs.py` around lines 21 - 29, Extract the repeated try/except/re-raise pattern into a reusable decorator (e.g., handle_route_errors) and apply it to all five endpoints in ban_list_configs.py: move the logic that catches Exception, re-raises HTTPException, and returns APIResponse.failure_response(error=_safe_error_message(exc)) into the decorator, then remove the try/except blocks from each endpoint function so they simply call ban_list_crud.create(session, payload, auth.organization_id, auth.project_id) and return APIResponse.success_response(...); ensure the decorator preserves function metadata (use functools.wraps) and references APIResponse and _safe_error_message so behavior remains identical.backend/app/tests/test_deps_multitenant.py (1)
100-104: Consider adding a test for the unconfigured-URL path (HTTP 500).
_fetch_tenant_from_backendraises a 500 whenKAAPI_BACKEND_CREDENTIAL_URLis empty. This path isn't covered. A quick test would prevent regressions if the guard is accidentally removed.💡 Example test
def test_validate_multitenant_key_missing_url_returns_500(monkeypatch): monkeypatch.setattr(settings, "KAAPI_BACKEND_CREDENTIAL_URL", "") with pytest.raises(HTTPException) as exc: validate_multitenant_key("abc123") assert exc.value.status_code == 500🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_deps_multitenant.py` around lines 100 - 104, Add a unit test that covers the unconfigured-backend-URL path by monkeypatching settings.KAAPI_BACKEND_CREDENTIAL_URL to an empty string and asserting validate_multitenant_key raises an HTTPException with status_code 500; specifically, create a test (e.g., test_validate_multitenant_key_missing_url_returns_500) that sets settings.KAAPI_BACKEND_CREDENTIAL_URL = "" via monkeypatch, calls validate_multitenant_key("abc123"), and asserts exc.value.status_code == 500 to exercise the _fetch_tenant_from_backend error path.backend/app/tests/test_banlists_integration.py (3)
40-79: Good validation coverage; consider adding a duplicate-creation test.The validation tests exercise the boundary constraints well using the imported constants. One gap: the CRUD layer handles
IntegrityErrorwith a 400 response for duplicate ban lists, but there's no test exercising that path. Consider adding a test that creates the same ban list twice and asserts a 400.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 40 - 79, Add a test in the TestCreateBanList suite that uses the existing create(...) helper to create a ban list twice and asserts the second response returns a 400; specifically call self.create(integration_client, "minimal") to create once, then call it again with the same payload and assert response.status_code == 400 to exercise the IntegrityError duplicate path handled by the CRUD layer (use the same integration_client and clear_database fixtures and reference BASE_URL only if needed).
199-207: Consider verifying the resource is actually removed after delete.
test_delete_successasserts the 200 response but doesn't confirm the item is gone. A follow-up GET asserting 404 would strengthen this test against a bug where the endpoint returns success without actually deleting.Suggested addition
assert response.status_code == 200 assert response.json()["success"] is True + + # Verify the resource is actually gone + get_resp = self.get(integration_client, ban_id) + assert get_resp.status_code == 404🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 199 - 207, Add a post-delete verification to TestDeleteBanList.test_delete_success: after calling self.delete(integration_client, ban_id) and asserting the 200/{"success": True} response, attempt to retrieve the same resource (e.g. call self.get(integration_client, ban_id) or re-run self.list(integration_client) and search for ban_id) and assert it is gone (expect a 404 from self.get or that ban_id is not present in the list). Update assertions to fail if the resource still exists.
133-138: Barenext()on generator will raise an opaqueStopIterationif seed data has no private items.If the seed data ever changes and no private ban list is present, this
next(...)raisesStopIterationinstead of a clear test failure. The same pattern appears at line 221. Providing a sentinel or wrapping in a helper improves debuggability.Suggested improvement
- private_ban_list = next( - item for item in list_resp.json()["data"] if not item["is_public"] - ) + private_items = [ + item for item in list_resp.json()["data"] if not item["is_public"] + ] + assert private_items, "Seed data must contain at least one private ban list" + private_ban_list = private_items[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 133 - 138, The test_get_wrong_owner_private uses bare next(...) on the generator which will raise StopIteration if no private item exists; change private_ban_list = next(... ) to use a sentinel (e.g. next(..., None)) and then assert private_ban_list is not None with a clear failure message before using ban_id; do the same for the other occurrence of bare next() in this test file so tests fail with an informative assertion instead of an opaque StopIteration (refer to symbols list_resp, private_ban_list, ban_id and the test_get_wrong_owner_private test to locate the change).
🤖 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/api/routes/ban_list_configs.py`:
- Around line 15-29: No changes required: the create_ban_list endpoint correctly
uses MultitenantAuthDep for tenant-scoped auth, preserves HTTP status codes by
re-raising HTTPException, and returns safe error strings via
_safe_error_message; leave the try/except and the use of ban_list_crud.create,
MultitenantAuthDep, HTTPException, and APIResponse.success_response /
APIResponse.failure_response as-is.
In `@backend/app/tests/test_banlists_integration.py`:
- Around line 219-235: The test_delete_wrong_owner function uses a bare
next(...) which can raise StopIteration; change the selection of
private_ban_list to use a safe lookup (e.g., next((item for item in
list_resp.json()["data"] if not item["is_public"]), None)) and then assert the
result is not None with a clear failure message before using ban_id, so the test
fails cleanly if no private list is present; update references to list() and
integration_client usage accordingly and keep ALT_API_KEY_999 as the auth key
for the delete call.
---
Nitpick comments:
In `@backend/app/api/deps.py`:
- Around line 80-122: _fetch_tenant_from_backend currently issues a synchronous
httpx.get per request (using settings.KAAPI_BACKEND_CREDENTIAL_URL) which can
add latency and backend dependency; add a short-lived TTL cache (e.g.,
cachetools.TTLCache keyed by the token) inside the module so the function first
checks the cache for a TenantContext before calling httpx.get, populate the
cache with the constructed TenantContext on successful responses, and keep
existing error handling (HTTPException/_unauthorized, parsing, and type checks)
intact; ensure cache timeout is configurable and use token as the cache key to
avoid leaking contexts between tokens.
- Around line 93-97: The except blocks currently re-raise HTTPException using
plain "raise HTTPException(...)" which causes implicit exception chaining and
can leak internal details; update each handler that catches httpx.RequestError
(the shown except httpx.RequestError and the similar handlers later) to re-raise
the HTTPException with "from None" (i.e., raise HTTPException(... ) from None)
so the original exception is suppressed in tracebacks/logs while preserving the
intended HTTP 503 behavior.
In `@backend/app/api/routes/ban_list_configs.py`:
- Around line 21-29: Extract the repeated try/except/re-raise pattern into a
reusable decorator (e.g., handle_route_errors) and apply it to all five
endpoints in ban_list_configs.py: move the logic that catches Exception,
re-raises HTTPException, and returns
APIResponse.failure_response(error=_safe_error_message(exc)) into the decorator,
then remove the try/except blocks from each endpoint function so they simply
call ban_list_crud.create(session, payload, auth.organization_id,
auth.project_id) and return APIResponse.success_response(...); ensure the
decorator preserves function metadata (use functools.wraps) and references
APIResponse and _safe_error_message so behavior remains identical.
In `@backend/app/tests/conftest.py`:
- Around line 91-107: The test override override_multitenant_key currently
strips an "ApiKey " prefix itself, which diverges from production behavior in
validate_multitenant_key and _fetch_tenant_from_backend that forward the raw
header as "ApiKey {token}"; update the override_multitenant_key function to
include a brief comment explaining this intentional simplification and the
semantic difference from validate_multitenant_key/_fetch_tenant_from_backend so
future readers know the test parses the header locally and may not mirror
production header forwarding (keep the override logic unchanged, just add the
explanatory comment near where
app.dependency_overrides[validate_multitenant_key] is set).
In `@backend/app/tests/test_banlist_configs.py`:
- Around line 106-124: Add a test that ensures an HTTPException raised by the
CRUD layer is propagated by the route handler: patch ban_list_crud.get to raise
fastapi.HTTPException, call delete_ban_list (the route handler) and assert it
raises HTTPException (use pytest.raises), and optionally assert
ban_list_crud.delete was not called; this verifies delete_ban_list does not
swallow HTTPException and re-raises it instead of returning
APIResponse.failure_response.
- Around line 48-59: The test test_create_calls_crud currently only asserts the
return value; update it to also assert that ban_list_crud.create was invoked
with the correct tenant args and payload — e.g. use
crud.create.assert_called_once_with(...) (or inspect crud.create.call_args) to
verify organization_id and project_id are passed from auth_context, the session
is mock_session, and the payload is create_payload when calling create_ban_list;
locate this in the test function test_create_calls_crud and add the single
assert_called_once_with (or equivalent) check referencing crud.create,
create_ban_list, mock_session, create_payload, and auth_context to match the
pattern used in test_update_success/test_delete_success.
In `@backend/app/tests/test_banlists_integration.py`:
- Around line 40-79: Add a test in the TestCreateBanList suite that uses the
existing create(...) helper to create a ban list twice and asserts the second
response returns a 400; specifically call self.create(integration_client,
"minimal") to create once, then call it again with the same payload and assert
response.status_code == 400 to exercise the IntegrityError duplicate path
handled by the CRUD layer (use the same integration_client and clear_database
fixtures and reference BASE_URL only if needed).
- Around line 199-207: Add a post-delete verification to
TestDeleteBanList.test_delete_success: after calling
self.delete(integration_client, ban_id) and asserting the 200/{"success": True}
response, attempt to retrieve the same resource (e.g. call
self.get(integration_client, ban_id) or re-run self.list(integration_client) and
search for ban_id) and assert it is gone (expect a 404 from self.get or that
ban_id is not present in the list). Update assertions to fail if the resource
still exists.
- Around line 133-138: The test_get_wrong_owner_private uses bare next(...) on
the generator which will raise StopIteration if no private item exists; change
private_ban_list = next(... ) to use a sentinel (e.g. next(..., None)) and then
assert private_ban_list is not None with a clear failure message before using
ban_id; do the same for the other occurrence of bare next() in this test file so
tests fail with an informative assertion instead of an opaque StopIteration
(refer to symbols list_resp, private_ban_list, ban_id and the
test_get_wrong_owner_private test to locate the change).
In `@backend/app/tests/test_deps_multitenant.py`:
- Around line 100-104: Add a unit test that covers the unconfigured-backend-URL
path by monkeypatching settings.KAAPI_BACKEND_CREDENTIAL_URL to an empty string
and asserting validate_multitenant_key raises an HTTPException with status_code
500; specifically, create a test (e.g.,
test_validate_multitenant_key_missing_url_returns_500) that sets
settings.KAAPI_BACKEND_CREDENTIAL_URL = "" via monkeypatch, calls
validate_multitenant_key("abc123"), and asserts exc.value.status_code == 500 to
exercise the _fetch_tenant_from_backend error path.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/api/routes/guardrails.py (1)
40-52:⚠️ Potential issue | 🟡 MinorUnhandled
HTTPExceptionfrom_resolve_ban_list_banned_wordsbreaks theAPIResponseenvelope.
_resolve_ban_list_banned_words(line 45) can raiseHTTPException(fromban_list_crud.get), which will bypass theAPIResponsewrapper and return a raw FastAPI error response. Every other error path in this handler returns a structuredAPIResponse. Wrap the call or let it flow through_validate_with_guard's try/except to keep responses consistent.Option: wrap in try/except alongside existing error handling
try: request_log = request_log_crud.create(payload) except ValueError: return APIResponse.failure_response(error="Invalid request_id") - _resolve_ban_list_banned_words(payload, session) - return _validate_with_guard( + try: + _resolve_ban_list_banned_words(payload, session) + except HTTPException as exc: + return APIResponse.failure_response(error=exc.detail) + + return _validate_with_guard( payload,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/guardrails.py` around lines 40 - 52, The call to _resolve_ban_list_banned_words can raise an HTTPException which bypasses the APIResponse envelope; update the handler to catch HTTPException (in the same try/except style as the ValueError around request_log_crud.create) and return APIResponse.failure_response with an appropriate error message (or propagate through _validate_with_guard by moving the call inside its try/except). Specifically, wrap the _resolve_ban_list_banned_words(payload, session) call in a try/except catching fastapi.HTTPException and return APIResponse.failure_response(error=...) so all error paths (including those from _resolve_ban_list_banned_words and request_log_crud.create) produce the structured APIResponse.
🧹 Nitpick comments (16)
backend/pyproject.toml (1)
36-45:pytest-asynciois unpinned, unlike every other dev dependency.All other entries in this group carry explicit version bounds. An unpinned
pytest-asynciocould silently pull in a future major version with breaking changes (e.g., the mode default changed between 0.21 → 0.23+). Consider adding a constraint consistent with the style used for the other packages.Suggested fix
- "pytest-asyncio", + "pytest-asyncio<1.0.0,>=0.23.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/pyproject.toml` around lines 36 - 45, The dev dependency list under dependency-groups has an unpinned entry pytest-asyncio; update the pytest-asyncio entry in the dependency-groups dev list to include explicit version bounds consistent with the other packages (e.g., add a compatible minimum and a <major upper bound such as "<1.0.0" or a more specific upper bound you prefer) so it won't silently pull breaking changes—modify the pytest-asyncio line in the dependency-groups dev array accordingly.backend/app/api/deps.py (2)
107-122: Response parsing trusts the first record blindly.
records[0]is used without checking that the first element is adict. If the backend ever returns a list of non-dict values (e.g.,[null]),record["organization_id"]raisesTypeError, which is caught and mapped to "Invalid API key" — so it's safe, but the error message is misleading.This is a minor robustness nit; the catch-all on line 121 ensures no crash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 107 - 122, The code currently takes records[0] without ensuring it's a dict which can produce a misleading TypeError; update the parsing in deps.py to validate that records is a non-empty list and that record := records[0] is an instance of dict before accessing keys, and if not raise _unauthorized("Invalid API key") (or a clearer message) so TenantContext(...) only runs with a proper dict containing "organization_id" and "project_id"; reference the variables records, record and the TenantContext constructor to locate where to add the type check and early raise.
87-97: Synchronoushttpx.getblocks a threadpool worker on every ban-list request.FastAPI runs sync dependencies in its threadpool, so this won't block the event loop, but each in-flight call occupies a worker for up to 5 seconds (the timeout). Under moderate concurrency this can exhaust the default threadpool.
Consider switching to an async implementation with
httpx.AsyncClient— this would let FastAPI handle it on the event loop directly.Also, the bare
raise HTTPException(...)inside theexcept httpx.RequestErrorblock (and similar patterns on lines 104–105, 121–122) implicitly chains the original exception. Addingfrom Nonesuppresses the noisy chain in server logs.♻️ Suggested async rewrite and exception chaining fix
-import httpx +import httpx +_http_client = httpx.AsyncClient(timeout=5) -def _fetch_tenant_from_backend(token: str) -> TenantContext: +async def _fetch_tenant_from_backend(token: str) -> TenantContext: if not settings.KAAPI_BACKEND_CREDENTIAL_URL: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="KAAPI_BACKEND_CREDENTIAL_URL is not configured", ) try: - response = httpx.get( + response = await _http_client.get( settings.KAAPI_BACKEND_CREDENTIAL_URL, headers={"X-API-KEY": f"ApiKey {token}"}, - timeout=5, ) except httpx.RequestError: raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Auth service unavailable", - ) + ) from NoneApply the same
from Noneon lines 104–105 and 121–122. Also makevalidate_multitenant_keyasync accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 87 - 97, Switch the synchronous httpx.get call to an async implementation: create and reuse an httpx.AsyncClient and await client.get(...) inside the dependency (make validate_multitenant_key async), so requests run on the event loop instead of blocking threadpool workers; also catch httpx.RequestError and re-raise HTTPException using "raise HTTPException(... ) from None" to suppress exception chaining. Apply the same "from None" change to the other HTTPException raises in this module that catch httpx.RequestError (the similar raise sites referenced in the comment) and ensure any callers are updated to await the now-async validate_multitenant_key.backend/app/core/validators/config/ban_list_safety_validator_config.py (1)
1-1: Uselistinstead oftyping.List(deprecated).
typing.Listis deprecated in favor of the built-inlisttype (Python 3.9+).Proposed fix
-from typing import List, Literal, Optional +from typing import Literal, OptionalAnd on line 12:
- banned_words: Optional[List[str]] = None + banned_words: Optional[list[str]] = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/validators/config/ban_list_safety_validator_config.py` at line 1, Replace the deprecated typing.List usage with the built-in list type: update the import line by removing List (keep Literal and Optional) and change all type annotations using List[...] in ban_list_safety_validator_config.py to use list[...] instead (e.g., List[str] -> list[str]); ensure any other references to typing.List are removed so only the modern built-in list is used.backend/app/alembic/versions/002_added_validator_log.py (1)
26-27: Consider adding indexes onorganization_idandproject_id.If the
validator_logtable will be queried or filtered by these columns (e.g., for tenant-scoped log retrieval), adding indexes would improve query performance as the table grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/alembic/versions/002_added_validator_log.py` around lines 26 - 27, The migration creates validator_log with organization_id and project_id but lacks indexes; add indexes for these columns to speed tenant-scoped queries by creating indexes in the upgrade and dropping them in downgrade. In the alembic migration (002_added_validator_log.py) add op.create_index calls for organization_id and project_id on the "validator_log" table (e.g., names like ix_validator_log_organization_id and ix_validator_log_project_id) and ensure corresponding op.drop_index calls exist in downgrade so rollbacks remove them.backend/app/tests/test_validate_with_guard.py (1)
19-21: Shared module-level mocks accumulate state across tests.
mock_request_log_crudandmock_validator_log_crudare instantiated once and reused across all tests without being reset. If any test inspects call counts or call args on these mocks, prior test runs will pollute the assertions.Consider converting these to
pytestfixtures withautouseor per-test scope so they're fresh for each test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_validate_with_guard.py` around lines 19 - 21, Mocks mock_request_log_crud and mock_validator_log_crud are module-level and retain state across tests; change them into per-test pytest fixtures (function scope) or autouse fixtures so each test gets a fresh MagicMock and fresh mock_request_log_id (uuid4) to avoid leaked call history; update test usages to accept the fixtures (names mock_request_log_crud, mock_validator_log_crud, mock_request_log_id) or provide a fixture that yields/reset mocks between tests, ensuring any existing references in test_validate_with_guard.py (and tests that import these names) are updated to use the fixture-provided objects.backend/app/tests/test_banlists_integration.py (2)
82-88: Magic number4couples the test to seed data cardinality.
assert len(data) == 4will break silently if a payload is added/removed fromBAN_LIST_PAYLOADS. Consider deriving the expected count from the seed data (e.g.,len(BAN_LIST_PAYLOADS)) or assertinglen(data) >= 1if the exact count isn't critical.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 82 - 88, The test TestListBanLists.test_list_success is using a hardcoded magic number (assert len(data) == 4) which couples it to seed data; update the assertion to derive the expected count from the seed payloads (e.g., assert len(data) == len(BAN_LIST_PAYLOADS)) or relax it to a minimum (e.g., assert len(data) >= 1) so the test won’t break if BAN_LIST_PAYLOADS changes; modify the assertion in test_list_success accordingly, referencing the BAN_LIST_PAYLOADS constant rather than the literal 4.
9-12: API key constants rely on implicit fall-through inconftest.override_multitenant_key.
DEFAULT_API_KEY = "org1_project1"doesn't match any explicit branch in the override function — it resolves todefault_scopevia the fall-throughreturn. While this works, it makes the mapping between API keys and tenant contexts non-obvious. Consider either adding an explicit branch for"org1_project1"in conftest, or using a name likeDEFAULT_API_KEY = "default"to make the implicit routing clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_integration.py` around lines 9 - 12, The test constant DEFAULT_API_KEY ("org1_project1") relies on implicit fall-through behavior in conftest.override_multitenant_key which maps it to default_scope; to make the mapping explicit, either add an explicit branch for "org1_project1" inside conftest.override_multitenant_key that returns the intended tenant scope, or change DEFAULT_API_KEY to a clearly routed value (e.g., "default") and update tests to use that; reference DEFAULT_API_KEY in backend/app/tests/test_banlists_integration.py and the override_multitenant_key function in conftest when making the change.backend/app/tests/test_banlist_configs.py (1)
48-58: Consider asserting thatcrud.createwas called with the expected arguments.Tests like
test_create_calls_crud,test_list_returns_data, andtest_get_successonly verify the response shape but don't assert that the CRUD methods were invoked with the correct session, IDs, and payload — unliketest_update_successandtest_delete_successwhich do verify call arguments. Addingassert_called_once_with(...)or inspectingcall_argswould strengthen these tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlist_configs.py` around lines 48 - 58, The test_create_calls_crud currently only asserts the return value; add an assertion that ban_list_crud.create was invoked with the expected arguments by calling crud.create.assert_called_once_with(payload=create_payload, session=mock_session, auth=auth_context) (or inspect crud.create.call_args) so the test verifies create_ban_list forwarded payload, session, and auth correctly; locate this change in the test function test_create_calls_crud referencing create_ban_list, ban_list_crud.create, mock_session, create_payload, auth_context, and sample_ban_list.backend/app/schemas/ban_list.py (1)
20-25: Consider adding length constraints toname,description, anddomainstring fields.These fields currently accept arbitrarily long strings. Adding
StringConstraints(max_length=...)orField(max_length=...)would protect against oversized inputs and align with the careful validation already applied tobanned_words.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/schemas/ban_list.py` around lines 20 - 25, Add string length constraints to the BanListBase model by updating the name, description, and domain fields to use Field (or StringConstraints) with sensible max_length values; modify the BanListBase class so the name, description, and domain attributes include Field(..., max_length=...) (or equivalent StringConstraints) to prevent arbitrarily long inputs and match the validation approach used for banned_words.backend/app/crud/ban_list.py (3)
1-1: Use built-inlistinstead oftyping.List.
typing.Listis deprecated since Python 3.9. Replace with built-inlistin the type hint on line 66.Proposed fix
-from typing import List, Optional +from typing import OptionalAnd on line 66:
- ) -> List[BanList]: + ) -> list[BanList]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/ban_list.py` at line 1, Replace the deprecated typing.List usage with the built-in list type in the type hint (the annotation currently importing and using List); remove List from the import list at the top (keep Optional if still used) and update the type annotation on the affected declaration (the one using List on line 66) to use builtin list[...] instead so the module no longer depends on typing.List.
42-58: Potential information leak: 404 is returned before ownership check.
get()returns a 404 if the object doesn't exist, and a 403 if the caller isn't the owner of a private resource. This ordering reveals whether a resource with a given UUID exists, even if the caller has no permission to access it. If an attacker enumerates UUIDs, they can distinguish "exists but forbidden" (403) from "doesn't exist" (404).For many internal APIs this is an acceptable trade-off, but if you want to avoid disclosing resource existence to unauthorized callers, consider returning 404 uniformly for both cases when the caller isn't the owner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/ban_list.py` around lines 42 - 58, The current get() returns 404 for missing BanList but 403 for unauthorized access, leaking existence; fix by masking authorization failures as 404: keep fetching obj = session.get(BanList, id) and still raise 404 if None, but when calling self.check_owner(obj, organization_id, project_id) (for require_owner or not obj.is_public) wrap that call in a try/except that catches HTTPException with status_code==403 and re-raises HTTPException(status_code=404, detail="Ban list not found") so unauthorized callers receive the same 404 response; reference the get() method and check_owner() call in BanList CRUD.
67-73: Use.is_(True)instead of== Truefor the SQLAlchemy boolean comparison.While
== Trueworks correctly in SQLAlchemy column expressions,.is_(True)is the idiomatic form that avoids the E712 lint warning and generatesIS TRUEin SQL, which is semantically more precise for nullable booleans.Proposed fix
stmt = select(BanList).where( ( (BanList.organization_id == organization_id) & (BanList.project_id == project_id) ) - | (BanList.is_public == True) + | (BanList.is_public.is_(True)) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/crud/ban_list.py` around lines 67 - 73, The boolean comparison on BanList.is_public should use SQLAlchemy's idiomatic .is_(True) to avoid E712 lint warnings and generate IS TRUE; update the select predicate building the stmt in the function handling organization_id and project_id to replace (BanList.is_public == True) with BanList.is_public.is_(True) while keeping the existing OR logic and the surrounding conditions that reference BanList, organization_id and project_id unchanged.backend/app/api/routes/guardrails.py (1)
85-99: Redundant type guard on line 90.After the
isinstance(validator, BanListSafetyValidatorConfig)check on line 87,validator.typeis always"ban_list"(it's aLiteral["ban_list"]). Thevalidator.type != BAN_LISTcheck is unreachable unlessBAN_LISTconstant is wrong, in which case you'd want a loud failure, not a silentcontinue.Simplify the guard
for validator in payload.validators: if not isinstance(validator, BanListSafetyValidatorConfig): continue - if validator.type != BAN_LIST or validator.banned_words is not None: + if validator.banned_words is not None: continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/guardrails.py` around lines 85 - 99, In _resolve_ban_list_banned_words, remove the redundant type check "validator.type != BAN_LIST" after the isinstance(validator, BanListSafetyValidatorConfig) guard; instead only skip when validator.banned_words is not None (i.e., use "if validator.banned_words is not None: continue"), or if you prefer a loud failure add an explicit assert/raise that validator.type == BAN_LIST immediately after the isinstance check; reference BanListSafetyValidatorConfig, validator.banned_words, and the BAN_LIST constant when making the change.backend/app/api/routes/ban_list_configs.py (1)
21-29: Consider extracting the repeated try/except/re-raise pattern.All five endpoints share the identical error-handling skeleton. A small decorator or helper would eliminate the duplication and make it trivial to adjust the policy later.
Example decorator approach
import functools def handle_exceptions(fn): `@functools.wraps`(fn) def wrapper(*args, **kwargs): try: return fn(*args, **kwargs) except HTTPException: raise except Exception as exc: return APIResponse.failure_response(error=_safe_error_message(exc)) return wrapperThen each endpoint just applies
@handle_exceptionsand keeps only the happy path.Also applies to: 38-46, 55-61, 71-83, 92-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_list_configs.py` around lines 21 - 29, Extract the repeated try/except/re-raise pattern into a reusable decorator (e.g., handle_exceptions) that catches Exception, re-raises HTTPException, and returns APIResponse.failure_response(error=_safe_error_message(exc)) for other errors; implement this decorator (using functools.wraps) and apply it to each endpoint function in ban_list_configs.py (the handlers that call ban_list_crud.create/ban_list_crud.update/ban_list_crud.delete etc.) so each handler only contains the happy-path logic and returns APIResponse.success_response on success.backend/app/tests/test_validator_configs_integration.py (1)
94-121: Hardcoded counts (4,3,2) are coupled to seed data — consider documenting or deriving them.If someone adds a validator to
seed_data.json, these assertions silently break. A brief comment noting the expected seed composition (e.g.,# seed_data.json seeds 4 validators for this org/project) would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_validator_configs_integration.py` around lines 94 - 121, The tests use hardcoded counts (4, 3, 2) tied to seed data; change them to derive expected counts from the actual seeded list returned by list_validators to avoid brittleness: call list_validators(integration_client) to fetch all validators, compute expected_count by filtering that full list by stage ("input") or type ("pii_remover") and assert len(data) == expected_count in test_list_validators_success, test_list_validators_filter_by_stage and test_list_validators_filter_by_type (or alternatively add a one-line comment in each test documenting the assumed seed composition if you prefer to keep hardcoded numbers). Ensure you reference the helper list_validators used in these tests when implementing the derivation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/app/api/routes/guardrails.py`:
- Around line 40-52: The call to _resolve_ban_list_banned_words can raise an
HTTPException which bypasses the APIResponse envelope; update the handler to
catch HTTPException (in the same try/except style as the ValueError around
request_log_crud.create) and return APIResponse.failure_response with an
appropriate error message (or propagate through _validate_with_guard by moving
the call inside its try/except). Specifically, wrap the
_resolve_ban_list_banned_words(payload, session) call in a try/except catching
fastapi.HTTPException and return APIResponse.failure_response(error=...) so all
error paths (including those from _resolve_ban_list_banned_words and
request_log_crud.create) produce the structured APIResponse.
---
Duplicate comments:
In `@backend/app/api/deps.py`:
- Around line 125-137: File ends without a trailing newline which triggers Ruff
W292; add a single newline character at the end of the file so the last line
(after the MultitenantAuthDep declaration) is terminated. Open the file
containing validate_multitenant_key and MultitenantAuthDep and ensure there is a
final '\n' after the last line, save the file, and re-run linting to confirm
W292 is resolved.
In `@backend/app/core/validators/config/ban_list_safety_validator_config.py`:
- Around line 21-25: The build() method currently allows self.banned_words to be
None and silently substitutes an empty list, hiding failures from
_resolve_ban_list_banned_words; update BanList builder in build() to defensively
check that self.banned_words is populated (not None and not empty if you require
at least one entry) and raise a clear exception (e.g., ValueError or custom
error) with context if resolution failed, so callers can detect the
misconfiguration early—refer to the build() method and the BanList construction
and use resolve_on_fail() as needed to include resolution context in the error
message.
In `@backend/app/crud/request_log.py`:
- Around line 14-15: The create method calls UUID(payload.request_id) which can
raise ValueError for invalid strings; update the code in create (and/or adjust
GuardrailRequest) so the request_id is validated: either change
GuardrailRequest.request_id to UUID to let Pydantic validate it, or wrap the
UUID(...) call in a try/except that catches ValueError and raises a clear, typed
error (e.g., a ValueError/HTTPException or custom ValidationError) with a
descriptive message before proceeding to construct RequestLog.
---
Nitpick comments:
In `@backend/app/alembic/versions/002_added_validator_log.py`:
- Around line 26-27: The migration creates validator_log with organization_id
and project_id but lacks indexes; add indexes for these columns to speed
tenant-scoped queries by creating indexes in the upgrade and dropping them in
downgrade. In the alembic migration (002_added_validator_log.py) add
op.create_index calls for organization_id and project_id on the "validator_log"
table (e.g., names like ix_validator_log_organization_id and
ix_validator_log_project_id) and ensure corresponding op.drop_index calls exist
in downgrade so rollbacks remove them.
In `@backend/app/api/deps.py`:
- Around line 107-122: The code currently takes records[0] without ensuring it's
a dict which can produce a misleading TypeError; update the parsing in deps.py
to validate that records is a non-empty list and that record := records[0] is an
instance of dict before accessing keys, and if not raise _unauthorized("Invalid
API key") (or a clearer message) so TenantContext(...) only runs with a proper
dict containing "organization_id" and "project_id"; reference the variables
records, record and the TenantContext constructor to locate where to add the
type check and early raise.
- Around line 87-97: Switch the synchronous httpx.get call to an async
implementation: create and reuse an httpx.AsyncClient and await client.get(...)
inside the dependency (make validate_multitenant_key async), so requests run on
the event loop instead of blocking threadpool workers; also catch
httpx.RequestError and re-raise HTTPException using "raise HTTPException(... )
from None" to suppress exception chaining. Apply the same "from None" change to
the other HTTPException raises in this module that catch httpx.RequestError (the
similar raise sites referenced in the comment) and ensure any callers are
updated to await the now-async validate_multitenant_key.
In `@backend/app/api/routes/ban_list_configs.py`:
- Around line 21-29: Extract the repeated try/except/re-raise pattern into a
reusable decorator (e.g., handle_exceptions) that catches Exception, re-raises
HTTPException, and returns
APIResponse.failure_response(error=_safe_error_message(exc)) for other errors;
implement this decorator (using functools.wraps) and apply it to each endpoint
function in ban_list_configs.py (the handlers that call
ban_list_crud.create/ban_list_crud.update/ban_list_crud.delete etc.) so each
handler only contains the happy-path logic and returns
APIResponse.success_response on success.
In `@backend/app/api/routes/guardrails.py`:
- Around line 85-99: In _resolve_ban_list_banned_words, remove the redundant
type check "validator.type != BAN_LIST" after the isinstance(validator,
BanListSafetyValidatorConfig) guard; instead only skip when
validator.banned_words is not None (i.e., use "if validator.banned_words is not
None: continue"), or if you prefer a loud failure add an explicit assert/raise
that validator.type == BAN_LIST immediately after the isinstance check;
reference BanListSafetyValidatorConfig, validator.banned_words, and the BAN_LIST
constant when making the change.
In `@backend/app/core/validators/config/ban_list_safety_validator_config.py`:
- Line 1: Replace the deprecated typing.List usage with the built-in list type:
update the import line by removing List (keep Literal and Optional) and change
all type annotations using List[...] in ban_list_safety_validator_config.py to
use list[...] instead (e.g., List[str] -> list[str]); ensure any other
references to typing.List are removed so only the modern built-in list is used.
In `@backend/app/crud/ban_list.py`:
- Line 1: Replace the deprecated typing.List usage with the built-in list type
in the type hint (the annotation currently importing and using List); remove
List from the import list at the top (keep Optional if still used) and update
the type annotation on the affected declaration (the one using List on line 66)
to use builtin list[...] instead so the module no longer depends on typing.List.
- Around line 42-58: The current get() returns 404 for missing BanList but 403
for unauthorized access, leaking existence; fix by masking authorization
failures as 404: keep fetching obj = session.get(BanList, id) and still raise
404 if None, but when calling self.check_owner(obj, organization_id, project_id)
(for require_owner or not obj.is_public) wrap that call in a try/except that
catches HTTPException with status_code==403 and re-raises
HTTPException(status_code=404, detail="Ban list not found") so unauthorized
callers receive the same 404 response; reference the get() method and
check_owner() call in BanList CRUD.
- Around line 67-73: The boolean comparison on BanList.is_public should use
SQLAlchemy's idiomatic .is_(True) to avoid E712 lint warnings and generate IS
TRUE; update the select predicate building the stmt in the function handling
organization_id and project_id to replace (BanList.is_public == True) with
BanList.is_public.is_(True) while keeping the existing OR logic and the
surrounding conditions that reference BanList, organization_id and project_id
unchanged.
In `@backend/app/schemas/ban_list.py`:
- Around line 20-25: Add string length constraints to the BanListBase model by
updating the name, description, and domain fields to use Field (or
StringConstraints) with sensible max_length values; modify the BanListBase class
so the name, description, and domain attributes include Field(...,
max_length=...) (or equivalent StringConstraints) to prevent arbitrarily long
inputs and match the validation approach used for banned_words.
In `@backend/app/tests/test_banlist_configs.py`:
- Around line 48-58: The test_create_calls_crud currently only asserts the
return value; add an assertion that ban_list_crud.create was invoked with the
expected arguments by calling
crud.create.assert_called_once_with(payload=create_payload,
session=mock_session, auth=auth_context) (or inspect crud.create.call_args) so
the test verifies create_ban_list forwarded payload, session, and auth
correctly; locate this change in the test function test_create_calls_crud
referencing create_ban_list, ban_list_crud.create, mock_session, create_payload,
auth_context, and sample_ban_list.
In `@backend/app/tests/test_banlists_integration.py`:
- Around line 82-88: The test TestListBanLists.test_list_success is using a
hardcoded magic number (assert len(data) == 4) which couples it to seed data;
update the assertion to derive the expected count from the seed payloads (e.g.,
assert len(data) == len(BAN_LIST_PAYLOADS)) or relax it to a minimum (e.g.,
assert len(data) >= 1) so the test won’t break if BAN_LIST_PAYLOADS changes;
modify the assertion in test_list_success accordingly, referencing the
BAN_LIST_PAYLOADS constant rather than the literal 4.
- Around line 9-12: The test constant DEFAULT_API_KEY ("org1_project1") relies
on implicit fall-through behavior in conftest.override_multitenant_key which
maps it to default_scope; to make the mapping explicit, either add an explicit
branch for "org1_project1" inside conftest.override_multitenant_key that returns
the intended tenant scope, or change DEFAULT_API_KEY to a clearly routed value
(e.g., "default") and update tests to use that; reference DEFAULT_API_KEY in
backend/app/tests/test_banlists_integration.py and the override_multitenant_key
function in conftest when making the change.
In `@backend/app/tests/test_validate_with_guard.py`:
- Around line 19-21: Mocks mock_request_log_crud and mock_validator_log_crud are
module-level and retain state across tests; change them into per-test pytest
fixtures (function scope) or autouse fixtures so each test gets a fresh
MagicMock and fresh mock_request_log_id (uuid4) to avoid leaked call history;
update test usages to accept the fixtures (names mock_request_log_crud,
mock_validator_log_crud, mock_request_log_id) or provide a fixture that
yields/reset mocks between tests, ensuring any existing references in
test_validate_with_guard.py (and tests that import these names) are updated to
use the fixture-provided objects.
In `@backend/app/tests/test_validator_configs_integration.py`:
- Around line 94-121: The tests use hardcoded counts (4, 3, 2) tied to seed
data; change them to derive expected counts from the actual seeded list returned
by list_validators to avoid brittleness: call
list_validators(integration_client) to fetch all validators, compute
expected_count by filtering that full list by stage ("input") or type
("pii_remover") and assert len(data) == expected_count in
test_list_validators_success, test_list_validators_filter_by_stage and
test_list_validators_filter_by_type (or alternatively add a one-line comment in
each test documenting the assumed seed composition if you prefer to keep
hardcoded numbers). Ensure you reference the helper list_validators used in
these tests when implementing the derivation.
In `@backend/pyproject.toml`:
- Around line 36-45: The dev dependency list under dependency-groups has an
unpinned entry pytest-asyncio; update the pytest-asyncio entry in the
dependency-groups dev list to include explicit version bounds consistent with
the other packages (e.g., add a compatible minimum and a <major upper bound such
as "<1.0.0" or a more specific upper bound you prefer) so it won't silently pull
breaking changes—modify the pytest-asyncio line in the dependency-groups dev
array accordingly.
dennyabrain
reviewed
Feb 17, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/api/deps.py (1)
93-97: Useraise … from Noneto suppress implicit exception chaining.In each
exceptblock that raises a newHTTPException, Python implicitly chains the original exception, producing noisy "During handling of the above exception…" tracebacks in logs. Usefrom None(orfrom excif you want explicit chaining) to keep error output clean.♻️ Example fix for all three sites
except httpx.RequestError: - raise HTTPException( + raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Auth service unavailable", - ) + ) from None ... except ValueError: - raise _unauthorized("Invalid API key") + raise _unauthorized("Invalid API key") from None ... except (KeyError, TypeError, ValueError): - raise _unauthorized("Invalid API key") + raise _unauthorized("Invalid API key") from NoneAlso applies to: 104-105, 121-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 93 - 97, The except blocks that catch httpx.RequestError and re-raise HTTPException (e.g., the handler in deps.py that currently does "except httpx.RequestError: raise HTTPException(...)" around the auth service calls) should suppress implicit exception chaining; update those raises to use "raise HTTPException(... ) from None" (apply the same change for the other two similar except blocks referenced at lines 104-105 and 121-122) so the original exception is not implicitly chained into the HTTPException traceback.
🤖 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/api/deps.py`:
- Around line 125-137: The file ends without a trailing newline; ensure the
source ends with a single newline character by adding a final newline at EOF so
validate_multitenant_key and the MultitenantAuthDep declaration remain unchanged
and the file ends with a proper newline.
---
Nitpick comments:
In `@backend/app/api/deps.py`:
- Around line 93-97: The except blocks that catch httpx.RequestError and
re-raise HTTPException (e.g., the handler in deps.py that currently does "except
httpx.RequestError: raise HTTPException(...)" around the auth service calls)
should suppress implicit exception chaining; update those raises to use "raise
HTTPException(... ) from None" (apply the same change for the other two similar
except blocks referenced at lines 104-105 and 121-122) so the original exception
is not implicitly chained into the HTTPException traceback.
AkhileshNegi
requested changes
Feb 17, 2026
| @@ -0,0 +1,41 @@ | |||
| from uuid import UUID | |||
Collaborator
There was a problem hiding this comment.
also API key should be passed in header instead of here
Collaborator
Author
There was a problem hiding this comment.
how do you want it to be done? I have included it in header only. You can check the code.
AkhileshNegi
requested changes
Feb 17, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (13)
backend/app/api/deps.py (2)
99-118: UnhandledJSONDecodeErrorfromresponse.json()on Line 102.If the upstream service returns a non-JSON 200 response,
response.json()raisesValueError/JSONDecodeError, which will bubble up as an unhandled 500 via the generic exception handler. While the test suite confirms this behavior, an explicit catch would give a more meaningful error message (e.g., 502 Bad Gateway) instead of a raw internal server error.♻️ Proposed: catch JSON parse errors explicitly
+ try: + data = response.json() + except (ValueError, KeyError): + raise HTTPException( + status_code=status.HTTP_502_BAD_GATEWAY, + detail="Invalid response from auth service", + ) from None - data = response.json() if not isinstance(data, dict) or data.get("success") is not True:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 99 - 118, Wrap the call to response.json() (the line assigning data) in a try/except that catches JSONDecodeError/ValueError, and when caught raise a 502-style error (e.g., use the existing error helper similar to _unauthorized but for bad gateway) with a clear message like "Upstream returned invalid JSON" — include the original exception message for context; then continue the existing validation logic that follows before returning TenantContext (organization_id, project_id).
87-97: Synchronoushttpx.getwithout connection pooling creates a new TCP connection per request.
httpx.get(...)is a convenience wrapper that opens and closes a fresh transport for every call. Under load this is wasteful and slow. Consider using a module-levelhttpx.Client(with connection pooling) or, ideally, switching tohttpx.AsyncClientwith anasync defdependency so FastAPI doesn't need a threadpool slot per auth check.Additionally, the
raise HTTPException(...)inside theexcept httpx.RequestErrorblock implicitly chains the original exception. Addfrom Noneto suppress internal details in tracebacks.♻️ Proposed: use a pooled client and suppress exception chain
+_auth_client = httpx.Client(timeout=5) + def _fetch_tenant_from_backend(token: str) -> TenantContext: if not settings.KAAPI_AUTH_URL: raise HTTPException( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail="KAAPI_AUTH_URL is not configured", ) try: - response = httpx.get( + response = _auth_client.get( f"{settings.KAAPI_AUTH_URL}/apikeys/verify", headers={"X-API-KEY": f"ApiKey {token}"}, - timeout=5, ) except httpx.RequestError: raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Auth service unavailable", - ) + ) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/deps.py` around lines 87 - 97, The current call to response = httpx.get(...) opens a fresh transport per request; change it to use a pooled client (create a module-level httpx.Client and call client.get(...) from the dependency) or, preferably, convert the dependency to async (make the function async, use a module-level httpx.AsyncClient and await client.get(...)) so you don't consume threadpool slots; ensure the client is created at module scope and closed on shutdown. Also update the except httpx.RequestError block to re-raise the HTTPException with from None (raise HTTPException(...) from None) to suppress exception chaining and avoid leaking internal trace details.backend/app/tests/conftest.py (1)
127-130:clear_databasefixture is a no-op — consider adding a docstring note or removing it.The fixture body is empty (
yield), and the docstring says cleanup is handled byclean_db. If it only exists for import compatibility in other test files, it's fine, but be aware it can silently mask missing cleanup in tests that assume it does something.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/conftest.py` around lines 127 - 130, The clear_database fixture is a no-op and should either be removed or made to delegate to the real cleanup fixture; update clear_database to accept the existing clean_db fixture (def clear_database(clean_db): yield) so tests importing clear_database still trigger the actual cleanup, or delete clear_database and update any imports/usages accordingly; also update the docstring to explicitly state it delegates to clean_db when keeping the fixture.backend/app/core/exception_handlers.py (1)
81-101: Consider deduplicating the two HTTP exception handlers.The
http_exception_handler(FastAPIHTTPException) andstarlette_http_exception_handler(StarletteHTTPException) have identical bodies. Since FastAPI'sHTTPExceptionis a subclass of Starlette's, this pattern is valid for catching exceptions from both routes and middleware, but the logic could be shared.♻️ Optional: extract shared handler body
+ async def _handle_http_exc(exc): + return JSONResponse( + status_code=exc.status_code, + content=APIResponse.failure_response( + _normalize_error_detail(exc.detail) + ).model_dump(), + headers=getattr(exc, "headers", None), + ) + `@app.exception_handler`(HTTPException) async def http_exception_handler(request: Request, exc: HTTPException): - return JSONResponse( - status_code=exc.status_code, - content=APIResponse.failure_response( - _normalize_error_detail(exc.detail) - ).model_dump(), - headers=exc.headers, - ) + return await _handle_http_exc(exc) `@app.exception_handler`(StarletteHTTPException) async def starlette_http_exception_handler( request: Request, exc: StarletteHTTPException ): - return JSONResponse( - status_code=exc.status_code, - content=APIResponse.failure_response( - _normalize_error_detail(exc.detail) - ).model_dump(), - headers=exc.headers, - ) + return await _handle_http_exc(exc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/core/exception_handlers.py` around lines 81 - 101, Both http_exception_handler and starlette_http_exception_handler have identical bodies; consolidate their logic by extracting the shared response creation into a single helper (e.g., a function build_error_response(request, exc) or a private async function) and have both exception handlers call that helper or register only one handler for the base exception type; update http_exception_handler and starlette_http_exception_handler to call the new helper which uses APIResponse.failure_response(_normalize_error_detail(exc.detail)).model_dump() and returns the JSONResponse with status_code and headers so the duplicated JSONResponse construction is centralized.backend/app/tests/test_banlists_api_integration.py (3)
133-138:next(...)without a default raisesStopIterationon no match, producing a confusing test failure.If the seed data has no private ban list, the error won't point to a failed assertion. The same pattern appears at line 221-222.
Proposed fix (example for line 135)
- private_ban_list = next( - item for item in list_resp.json()["data"] if not item["is_public"] - ) + private_items = [ + item for item in list_resp.json()["data"] if not item["is_public"] + ] + assert private_items, "Seed data must include at least one private ban list" + private_ban_list = private_items[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api_integration.py` around lines 133 - 138, The test test_get_wrong_owner_private uses next(...) without a default which raises StopIteration if no private ban list exists; update the selection logic (the call to next(...) that assigns private_ban_list) to supply a default (e.g., None) or guard via a conditional and then assert a clear failure if no private item is found before dereferencing ban_id; apply the same fix to the identical pattern later (lines around where ban_id is set) so the test fails with an explicit assertion message rather than a StopIteration exception.
82-88: Magic number4couples the test to seed data internals.
assert len(data) == 4will break silently if seed data changes. Consider defining this expected count as a constant inseed_data.pyalongside the seed definitions, or dynamically deriving it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api_integration.py` around lines 82 - 88, The test TestListBanLists.test_list_success currently asserts a hard-coded length (assert len(data) == 4); replace this magic number by referencing a canonical source in the seed data instead: either add an exported constant (e.g., EXPECTED_BANLIST_COUNT) to seed_data.py next to the seed definitions and use that in the test, or compute the expected count dynamically from the seed data helper (call the same seeding function/collection used by tests) and assert len(data) == EXPECTED_BANLIST_COUNT; update the test_list_success to import and use that symbol (or call the seed helper) so the assertion no longer depends on a literal 4.
9-12: Hard-coded API keys should be sourced from a shared constants/seed module.
DEFAULT_API_KEY,ALT_API_KEY_999, andALT_API_KEY_2are string literals tied to the auth/seed setup. If the seed data or auth mock changes, these will silently break. Consider co-locating them with the seed data definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api_integration.py` around lines 9 - 12, Replace the hard-coded API key literals DEFAULT_API_KEY, ALT_API_KEY_999, and ALT_API_KEY_2 in the test with imports from the project’s shared seed/constants module that defines the auth/seed data (e.g., use the exported constants for those API keys), updating the top of backend/app/tests/test_banlists_api_integration.py to import the named constants and remove the string literals so tests follow the canonical seed values and won’t break if the seed changes.backend/app/api/routes/ban_lists.py (3)
38-45: Parameteridshadows the Python built-in.Using
idas a parameter name shadows the built-inid()function. This is a minor style concern — consider renaming toban_list_idfor clarity, though it's not strictly required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_lists.py` around lines 38 - 45, The route parameter shadows Python's built-in id(); rename the parameter in get_ban_list from id to ban_list_id and update all references: change the decorator path to "/{ban_list_id}" (or use Path alias) and pass ban_list_id into ban_list_crud.get(session, ban_list_id, auth.organization_id, auth.project_id) so the function name get_ban_list and other logic remain the same but the built-in is no longer shadowed.
14-23: POST endpoint returns 200 instead of 201.The create endpoint defaults to HTTP 200. For REST convention, a resource creation should return 201 Created. Add
status_code=201to the decorator.Proposed fix
-@router.post("/", response_model=APIResponse[BanListResponse]) +@router.post("/", response_model=APIResponse[BanListResponse], status_code=201)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_lists.py` around lines 14 - 23, The create_ban_list POST handler currently returns HTTP 200 by default; update the router.post decorator for create_ban_list to explicitly set status_code=201 so the endpoint returns 201 Created on successful creation (keep returning APIResponse.success_response as before). Locate the router.post(...) decorator above the create_ban_list function and add the status_code=201 parameter in that decorator declaration.
26-35: List endpoint lacks pagination.Returning all matching ban lists without
limit/offset(or cursor) parameters will become a problem as data grows. Consider adding pagination support now to avoid a breaking API change later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/ban_lists.py` around lines 26 - 35, The list_ban_lists endpoint returns all results without pagination; add limit and offset (or cursor) query params to list_ban_lists with sensible defaults and validation, pass those parameters into ban_list_crud.list (update its signature to accept limit/offset or cursor), and change the returned payload to a paginated shape (e.g., APIResponse wrapping items: list[BanListResponse] plus pagination metadata like total/limit/offset or next_cursor) so callers can page through results; update any tests and callers to use the new paginated response shape.backend/app/tests/test_banlists_api.py (3)
1-1: Unused import:uuid.The
uuidmodule is imported but never used in this file.Proposed fix
-import uuid🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api.py` at line 1, The import of the uuid module (symbol: uuid) in test_banlists_api.py is unused; remove the unused import line "import uuid" from the file so the test module no longer contains an unused dependency.
48-58:test_create_calls_cruddoesn't verify CRUD was called with correct arguments.The test name says "calls crud" but only checks the return value. Consider asserting
crud.create.assert_called_once_with(...)liketest_update_successdoes, to verifyorganization_idandproject_idare forwarded correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api.py` around lines 48 - 58, Update the test_create_calls_crud test to assert that ban_list_crud.create was invoked with the correct arguments: call create_ban_list and then call crud.create.assert_called_once_with(...) (or the exact arg assertion pattern used in test_update_success) ensuring the payload, session, and auth-derived organization_id and project_id are forwarded (reference test name test_create_calls_crud, the create_ban_list function, and the mock target ban_list_crud.create for locating where to add the assertion).
61-70:test_list_returns_datadoesn't verify thedomainparameter is forwarded.The list route accepts an optional
domainfilter. Consider adding a test case that passesdomainand verifiescrud.listreceives it, to ensure filter forwarding works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/tests/test_banlists_api.py` around lines 61 - 70, Add an assertion to verify the optional domain filter is forwarded: update test_list_returns_data to call list_ban_lists with a domain argument (e.g., domain="example.com") and assert that the patched ban_list_crud.list was called with that domain value (inspect the mock's call args). Locate references to test_list_returns_data, list_ban_lists, and ban_list_crud.list in the test and add the domain parameter to the function call and an assertion like crud.list.assert_called_with(..., domain="example.com") ensuring other required args (session/auth) match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/api/deps.py`:
- Around line 99-118: Wrap the call to response.json() (the line assigning data)
in a try/except that catches JSONDecodeError/ValueError, and when caught raise a
502-style error (e.g., use the existing error helper similar to _unauthorized
but for bad gateway) with a clear message like "Upstream returned invalid JSON"
— include the original exception message for context; then continue the existing
validation logic that follows before returning TenantContext (organization_id,
project_id).
- Around line 87-97: The current call to response = httpx.get(...) opens a fresh
transport per request; change it to use a pooled client (create a module-level
httpx.Client and call client.get(...) from the dependency) or, preferably,
convert the dependency to async (make the function async, use a module-level
httpx.AsyncClient and await client.get(...)) so you don't consume threadpool
slots; ensure the client is created at module scope and closed on shutdown. Also
update the except httpx.RequestError block to re-raise the HTTPException with
from None (raise HTTPException(...) from None) to suppress exception chaining
and avoid leaking internal trace details.
In `@backend/app/api/routes/ban_lists.py`:
- Around line 38-45: The route parameter shadows Python's built-in id(); rename
the parameter in get_ban_list from id to ban_list_id and update all references:
change the decorator path to "/{ban_list_id}" (or use Path alias) and pass
ban_list_id into ban_list_crud.get(session, ban_list_id, auth.organization_id,
auth.project_id) so the function name get_ban_list and other logic remain the
same but the built-in is no longer shadowed.
- Around line 14-23: The create_ban_list POST handler currently returns HTTP 200
by default; update the router.post decorator for create_ban_list to explicitly
set status_code=201 so the endpoint returns 201 Created on successful creation
(keep returning APIResponse.success_response as before). Locate the
router.post(...) decorator above the create_ban_list function and add the
status_code=201 parameter in that decorator declaration.
- Around line 26-35: The list_ban_lists endpoint returns all results without
pagination; add limit and offset (or cursor) query params to list_ban_lists with
sensible defaults and validation, pass those parameters into ban_list_crud.list
(update its signature to accept limit/offset or cursor), and change the returned
payload to a paginated shape (e.g., APIResponse wrapping items:
list[BanListResponse] plus pagination metadata like total/limit/offset or
next_cursor) so callers can page through results; update any tests and callers
to use the new paginated response shape.
In `@backend/app/core/exception_handlers.py`:
- Around line 81-101: Both http_exception_handler and
starlette_http_exception_handler have identical bodies; consolidate their logic
by extracting the shared response creation into a single helper (e.g., a
function build_error_response(request, exc) or a private async function) and
have both exception handlers call that helper or register only one handler for
the base exception type; update http_exception_handler and
starlette_http_exception_handler to call the new helper which uses
APIResponse.failure_response(_normalize_error_detail(exc.detail)).model_dump()
and returns the JSONResponse with status_code and headers so the duplicated
JSONResponse construction is centralized.
In `@backend/app/tests/conftest.py`:
- Around line 127-130: The clear_database fixture is a no-op and should either
be removed or made to delegate to the real cleanup fixture; update
clear_database to accept the existing clean_db fixture (def
clear_database(clean_db): yield) so tests importing clear_database still trigger
the actual cleanup, or delete clear_database and update any imports/usages
accordingly; also update the docstring to explicitly state it delegates to
clean_db when keeping the fixture.
In `@backend/app/tests/test_banlists_api_integration.py`:
- Around line 133-138: The test test_get_wrong_owner_private uses next(...)
without a default which raises StopIteration if no private ban list exists;
update the selection logic (the call to next(...) that assigns private_ban_list)
to supply a default (e.g., None) or guard via a conditional and then assert a
clear failure if no private item is found before dereferencing ban_id; apply the
same fix to the identical pattern later (lines around where ban_id is set) so
the test fails with an explicit assertion message rather than a StopIteration
exception.
- Around line 82-88: The test TestListBanLists.test_list_success currently
asserts a hard-coded length (assert len(data) == 4); replace this magic number
by referencing a canonical source in the seed data instead: either add an
exported constant (e.g., EXPECTED_BANLIST_COUNT) to seed_data.py next to the
seed definitions and use that in the test, or compute the expected count
dynamically from the seed data helper (call the same seeding function/collection
used by tests) and assert len(data) == EXPECTED_BANLIST_COUNT; update the
test_list_success to import and use that symbol (or call the seed helper) so the
assertion no longer depends on a literal 4.
- Around line 9-12: Replace the hard-coded API key literals DEFAULT_API_KEY,
ALT_API_KEY_999, and ALT_API_KEY_2 in the test with imports from the project’s
shared seed/constants module that defines the auth/seed data (e.g., use the
exported constants for those API keys), updating the top of
backend/app/tests/test_banlists_api_integration.py to import the named constants
and remove the string literals so tests follow the canonical seed values and
won’t break if the seed changes.
In `@backend/app/tests/test_banlists_api.py`:
- Line 1: The import of the uuid module (symbol: uuid) in test_banlists_api.py
is unused; remove the unused import line "import uuid" from the file so the test
module no longer contains an unused dependency.
- Around line 48-58: Update the test_create_calls_crud test to assert that
ban_list_crud.create was invoked with the correct arguments: call
create_ban_list and then call crud.create.assert_called_once_with(...) (or the
exact arg assertion pattern used in test_update_success) ensuring the payload,
session, and auth-derived organization_id and project_id are forwarded
(reference test name test_create_calls_crud, the create_ban_list function, and
the mock target ban_list_crud.create for locating where to add the assertion).
- Around line 61-70: Add an assertion to verify the optional domain filter is
forwarded: update test_list_returns_data to call list_ban_lists with a domain
argument (e.g., domain="example.com") and assert that the patched
ban_list_crud.list was called with that domain value (inspect the mock's call
args). Locate references to test_list_returns_data, list_ban_lists, and
ban_list_crud.list in the test and add the domain parameter to the function call
and an assertion like crud.list.assert_called_with(..., domain="example.com")
ensuring other required args (session/auth) match.
AkhileshNegi
requested changes
Feb 17, 2026
AkhileshNegi
approved these changes
Feb 17, 2026
AkhileshNegi
approved these changes
Feb 17, 2026
rkritika1508
added a commit
that referenced
this pull request
Feb 18, 2026
Co-authored-by: AkhileshNegi <akhileshnegi.an3@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Target issue is #33.
Explain the motivation for making this change. What existing problem does the pull request solve?
Currently, we have no way to manage ban list for each NGOs. So, we want to build APIs which manage ban list and allow other NGOs to use them as well.
The following APIs will be added -
File-by-File Breakdown
APIs
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
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit
New Features
Documentation