-
Notifications
You must be signed in to change notification settings - Fork 2
Added validator logs and organized code #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds per-validator logging and request logging models: migration updates Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Guardrails API
participant Guard as Guard (validators)
participant LogCrud as ValidatorLogCrud
participant DB as Database
Client->>API: call run_input_guardrails / run_output_guardrails(request)
API->>API: derive and validate request_id
alt invalid request_id
API-->>Client: return failure (invalid request id)
else valid request_id
API->>LogCrud: instantiate ValidatorLogCrud(session)
API->>Guard: _validate_with_guard(payload, validator_log_crud, request_log_id)
Guard->>Guard: execute validators -> produce results + history
Guard-->>API: validation result + history
API->>LogCrud: add_validator_logs(guard, request_log_id, validator_log_crud)
LogCrud->>DB: create(ValidatorLog entries)
DB-->>LogCrud: persisted entries
alt validation success
API-->>Client: return success (sorted output, request_id)
else validation failure
API-->>Client: return failure (generic error, request_id)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/guardrails.py`:
- Around line 24-27: The code calls UUID(payload.request_id) which can raise
ValueError and yield a 500; update the guardrails route to validate request_id
and return a 4xx instead: either change the request Pydantic model to use
typing.UUID for request_id (so FastAPI will validate automatically) or catch
ValueError around the UUID(...) conversion in the handler that uses
RequestLogCrud and ValidatorLogCrud, and return an appropriate HTTP 400/422
response with a clear message; locate the UUID(...) usage and surrounding logic
where RequestLogCrud.create and ValidatorLogCrud are called to apply the change.
- Around line 146-160: iteration.outputs.validator_logs handling writes the
guardrail outcome as-is, but Guardrails returns lowercase "pass"/"fail" which
fails the ValidatorOutcome enum; update the persistence call by normalizing
result.outcome to uppercase and converting it to the ValidatorOutcome enum
before calling validator_log_crud.create. Specifically, where FailResult and
result are used in the loop (iteration.outputs.validator_logs, FailResult,
result.outcome), transform outcome via ValidatorOutcome(result.outcome.upper())
and pass that value into validator_log_crud.create's outcome parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/api/routes/guardrails.py`:
- Around line 164-192: In add_validator_logs, convert non-string validator
values to safe strings before constructing the ValidatorLog: when building
ValidatorLog (class ValidatorLog) use a defensive conversion for
log.value_before_validation and log.value_after_validation (e.g., coerce None to
empty string and wrap non-str values with str() or json.dumps for complex
objects) so input and output are always non-null strings; keep existing
error_message logic and call validator_log_crud.create as before. Also ensure
the file ends with a trailing newline.
♻️ Duplicate comments (1)
backend/app/crud/validator_log.py (1)
8-17: LGTM!The
ValidatorLogCrudclass follows a consistent pattern withRequestLogCrudand correctly handles the database operations for persisting validator logs. Note: Add a trailing newline at end of file per linting rules.
🧹 Nitpick comments (1)
backend/app/crud/validator_log.py (1)
1-1: Remove unused imports.
UUIDanduuid4are imported but never used in this file.♻️ Proposed fix
-from uuid import UUID, uuid4 - from sqlmodel import Session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/app/models/logging/request.py`:
- Around line 66-68: The file ends without a trailing newline causing Ruff W292;
add a single newline character at EOF so the class definition for
RequestLogUpdate (response_text: str, response_id: UUID) is followed by a
newline, then save the file to satisfy the linter.
♻️ Duplicate comments (1)
backend/app/models/logging/validator.py (1)
63-63: Add trailing newline (W292).File is missing a newline at EOF.
🧹 Nitpick comments (3)
backend/app/models/logging/request.py (1)
59-63:updated_atwon't auto-update on record modifications.The
default_factory=nowonly sets the value at insert time. To automatically update this timestamp when the record is modified, use SQLAlchemy'sonupdateparameter.♻️ Suggested fix
updated_at: datetime = Field( default_factory=now, nullable=False, - sa_column_kwargs={"comment": "Timestamp when the entry was last updated"}, + sa_column_kwargs={ + "comment": "Timestamp when the entry was last updated", + "onupdate": now, + }, )backend/app/models/logging/validator.py (2)
38-46: Consider adding explicitdefault=Nonefor nullable fields.While SQLModel may infer the default for
str | Nonetypes, explicitly settingdefault=Noneimproves clarity and ensures consistent behavior.♻️ Suggested fix
- output: str | None = Field( + output: str | None = Field( + default=None, nullable=True, sa_column_kwargs={"comment": "Output message post validation"}, ) - error: str | None = Field( + error: str | None = Field( + default=None, nullable=True, sa_column_kwargs={"comment": "Error message if the validator throws an exception"}, )
59-63:updated_atwon't auto-update on record modifications.Same as in
request.py: useonupdateinsa_column_kwargsto automatically update this timestamp when the record is modified.♻️ Suggested fix
updated_at: datetime = Field( default_factory=now, nullable=False, - sa_column_kwargs={"comment": "Timestamp when the entry was last updated"}, + sa_column_kwargs={ + "comment": "Timestamp when the entry was last updated", + "onupdate": now, + }, ) +
Summary
Target issue is #19.
Explain the motivation for making this change. What existing problem does the pull request solve?
Currently, we are using adding request logs, which work at the API level. Now, we will also have validator logs. So, for each validator that is triggered by the API, we will have the input_before_validation, input_after_validation, outcome, error, validation type, etc. This will make the logs more comprehensive and useful for NGOs to understand where the guardrails are failing and what are the reasons for the failure.
The
guard.historyparameter contains the validator logs which we are using.Adding a sample schema from where we are extracting the logs -
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.