[NA] [BE/FE] feat: guardrails improvements — CPU image, dashboard breakdown, alert type filter#7506
[NA] [BE/FE] feat: guardrails improvements — CPU image, dashboard breakdown, alert type filter#7506alexkuzmik wants to merge 8 commits into
Conversation
…mode Split the guardrails backend into a GPU Dockerfile (unchanged, NVIDIA CUDA base) and a new CPU Dockerfile.cpu (slim, multi-arch, no GPU/NVIDIA toolkit), so the service builds and runs on CPU-only and arm64 hosts. Docker Compose now supports both modes: guardrails-backend (GPU, with an NVIDIA device reservation) under the guardrails profile, and a new guardrails-backend-cpu service under the guardrails-cpu profile. opik.sh gains a --guardrails-cpu flag alongside --guardrails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ails chart Add a GUARDRAIL_NAME breakdown field, compatible only with the existing GUARDRAILS_FAILED_COUNT metric, so the "Failed guardrails" chart can be grouped into one series per guardrail name (PII, Topic, ...). The trace-scoped breakdown query already joins the guardrails table, so grouping maps to ifNull(g.name, 'Unknown') with no query changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a filter:guardrail_type trigger config to the guardrails alert so an alert can target specific guardrail types (PII, Topic) instead of firing on any failure. Empty selection preserves the current "fire on any type" behavior. Backend: new AlertTriggerConfigType.FILTER_GUARDRAIL_TYPE + config-type enum migration; AlertEventEvaluationService matches the failed-guardrail payload against the configured types (OR; empty = all). Frontend (v2): guardrail-type multiselect on the guardrails trigger, wired through the form/payload mapping and schema. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
⏱️ pre-commit per-hook timing
⏭️ 28 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| const config = triggerConfigs?.find( | ||
| (c) => c.type === ALERT_TRIGGER_CONFIG_TYPE["filter:guardrail_type"], | ||
| ); |
There was a problem hiding this comment.
Drops extra guardrail filters
getGuardrailTypesFromTriggerConfigs() uses find(...), so reopening an alert with multiple filter:guardrail_type configs drops every match after the first and a save writes back only that subset, unlike AlertEventEvaluationService.matchesGuardrailTypeFilter() which ORs them all — should we collect every matching config here?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/v2/pages/AlertsPage/AddEditAlertPage/helpers.ts around lines
215-229, update `getGuardrailTypesFromTriggerConfigs()` to collect every
`filter:guardrail_type` config instead of using `.find(...)` (which only reads the first
match). Refactor the logic to iterate over all matching `triggerConfigs` entries,
extract each `config_value.guardrail_types` string, split/trim/uppercase each, and
union/deduplicate the resulting GuardrailTypes before returning. This ensures reopening
an alert with multiple guardrail filter configs doesn’t silently drop configs,
matching the OR/flatten behavior in `matchesGuardrailTypeFilter()`.
There was a problem hiding this comment.
Commit f751f35 addressed this comment by changing getGuardrailTypesFromTriggerConfigs() to iterate over all filter:guardrail_type trigger configs instead of using .find(...). It now flattens, trims, uppercases, and deduplicates every matching guardrail_types entry before returning.
| Set<String> configuredTypes = CollectionUtils.emptyIfNull(alert.triggers()).stream() | ||
| .filter(trigger -> trigger.eventType() == alertEvent.eventType()) | ||
| .flatMap(trigger -> CollectionUtils.emptyIfNull(trigger.triggerConfigs()).stream()) | ||
| .filter(config -> config.type() == AlertTriggerConfigType.FILTER_GUARDRAIL_TYPE) | ||
| .map(config -> config.configValue() != null | ||
| ? config.configValue().get(AlertTriggerConfig.GUARDRAIL_TYPES_CONFIG_KEY) | ||
| : null) | ||
| .filter(StringUtils::isNotBlank) | ||
| .flatMap(value -> Arrays.stream(value.split(","))) | ||
| .map(String::trim) | ||
| .filter(StringUtils::isNotBlank) | ||
| .map(type -> type.toUpperCase(java.util.Locale.ROOT)) |
There was a problem hiding this comment.
matchesGuardrailTypeFilter() unions all filter:guardrail_type values across every TRACE_GUARDRAILS_TRIGGERED trigger into one configuredTypes set, so a single filtered trigger makes the whole alert behave as filtered and events that should satisfy an unfiltered trigger stop firing — should we evaluate the type filter per trigger and short-circuit with anyMatch (treating absent/blank guardrail_type as wildcard)?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/java/com/comet/opik/domain/alerts/AlertEventEvaluationService.java
around lines 68-95, refactor matchesGuardrailTypeFilter(AlertEvent alertEvent, Alert
alert) to evaluate each TRACE_GUARDRAILS_TRIGGERED trigger independently instead of
unioning all filter:guardrail_type values into a single configuredTypes set. For each
trigger whose eventType() matches the incoming eventType, derive its guardrail_type
filter: if that filter is null/absent/blank, return true immediately for that trigger
(wildcard); otherwise parse the comma-separated types and check the guardrails payload
against those types. Use anyMatch so the alert fires if ANY trigger accepts the payload.
Preserve existing payload-shape handling, but base the final decision on per-trigger
matching rather than a single aggregated set.
There was a problem hiding this comment.
Commit f751f35 addressed this comment by switching guardrail filtering from one aggregated configuredTypes set to per-trigger evaluation with anyMatch. It now treats a trigger with no filter:guardrail_type config as a wildcard, so one filtered trigger no longer suppresses an unfiltered sibling trigger.
| assertThat(response.results()).allSatisfy( | ||
| result -> assertThat(result.name()).isIn("TOPIC", "PII")); | ||
| } |
There was a problem hiding this comment.
Missing series still passes
allSatisfy(result -> assertThat(result.name()).isIn("TOPIC", "PII")) only checks membership per item, so a response missing one guardrail series still passes — should we assert the exact name set/size, or compare the whole list, to cover the new GUARDRAIL_NAME breakdown?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/ProjectMetricsWithBreakdownResourceTest.java
around lines 706-736, in the `happyPathGroupByGuardrailName` test, the current assertion
`results().allSatisfy(... isIn("TOPIC", "PII"))` is too weak because it allows the
response to include only one of the two expected series. Refactor the assertions to
verify the exact expected coverage for the GUARDRAIL_NAME breakdown by asserting the
results count/size is exactly 2 and that the set of `result.name()` values equals
exactly {"TOPIC", "PII"} (or compare the full results list if a stable order is
guaranteed).
There was a problem hiding this comment.
Commit f751f35 addressed this comment by replacing the weak allSatisfy(... isIn(...)) assertion with extracting(result -> result.name()).containsExactlyInAnyOrder(GuardrailType.TOPIC.name(), GuardrailType.PII.name()), which verifies both the exact set of series names and their count. It also made the test data deterministic so both guardrail types are always created.
| 'filter:guardrail_type' | ||
| ) NOT NULL; | ||
|
|
||
| --rollback empty |
There was a problem hiding this comment.
Unreversible enum change lacks rollback guidance
filter:guardrail_type is a new persisted ENUM in alert_trigger_configs.config_type, so --rollback empty leaves no recovery path once rows use it — should we replace it with a forward-only rollback note that says reverting requires restoring from backup or deleting/updating those rows first?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/main/resources/liquibase/db-app-state/migrations/000090_add_filter_guardrail_type_config_type.sql
around lines 1-15, for the Liquibase changeset
`alexkuzmik:000090_add_filter_guardrail_type_config_type`, the `--rollback empty` is
unsafe/documentation-poor because reverting after data is written with
`filter:guardrail_type` is not automatically correct. Replace the rollback section with
an explicit rollback warning/comment stating that rollback is forward-only: reverting
requires restoring from backup or manually deleting/updating rows that already use
`filter:guardrail_type` before attempting to change the ENUM back, and that no automatic
SQL rollback will be provided. Ensure the comments are clear and remain within the same
changeset so Liquibase operators see the warning during rollback attempts.
There was a problem hiding this comment.
Commit f751f35 addressed this comment by adding a forward-only rollback warning directly above --rollback empty. The new note explains that reverting after rows use filter:guardrail_type is unsafe and that recovery requires deleting/migrating those rows or restoring the previous enum from backup.
| FROM python:3.10-slim-bookworm | ||
|
|
||
| ENV PYTHONUNBUFFERED=1 \ | ||
| DEBIAN_FRONTEND=noninteractive \ | ||
| TZ=UTC \ | ||
| OPIK_GUARDRAILS_DEVICE=cpu | ||
|
|
||
| # tini for signal handling / zombie reaping, wget for the healthcheck. | ||
| # DL3008: Debian apt packages track a rolling security channel; pinning exact | ||
| # versions here would rot as the base image's repos are updated. | ||
| # hadolint ignore=DL3008 | ||
| RUN apt-get update && apt-get install -y --no-install-recommends \ |
There was a problem hiding this comment.
CPU image regressions go untested
Dockerfile.cpu introduces a new CPU-only runtime path, so a bad torch CPU wheel pin, USER 1001 permission change, or model-download contract could break startup and ship unnoticed — should we add a smoke test that builds Dockerfile.cpu and boots the container in CI?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-guardrails-backend/Dockerfile.cpu around lines 7-47 (the CPU-only build steps
that install torch and download models, then run as USER 1001), add a targeted smoke
test in CI that validates the image can build and boot successfully. Create a CI job
that runs `docker build -f apps/opik-guardrails-backend/Dockerfile.cpu` and then `docker
run` the resulting image with the port mapping for 5000, waits until the server responds
(preferably by curling the existing health/ready endpoint used elsewhere in the repo),
and fails if the container doesn’t start or the endpoint doesn’t return success
within a timeout. If there is already a healthcheck/endpoint helper in the repository,
reuse it; otherwise add a minimal script/step that checks basic liveness over HTTP to
ensure this Dockerfile’s boot contract is enforced.
There was a problem hiding this comment.
Deferring to a follow-up. A CI job that builds Dockerfile.cpu and boots the container is worthwhile but is a separate CI/infra change (build time + runner setup) beyond this PR's scope. The CPU image was built and booted end-to-end locally for this PR. Tracking separately.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Thanks for the context — noted, I'll save this to memory once the PR is merged.
| RUN pip install --no-cache-dir -U "pip>=24.0" && \ | ||
| pip install --no-cache-dir --index-url https://download.pytorch.org/whl/cpu torch==2.6.0 && \ | ||
| pip install --no-cache-dir --disable-pip-version-check -r requirements.txt |
There was a problem hiding this comment.
Builds drift across rebuilds
pip install -U "pip>=24.0" floats to whatever pip version is newest at build time, so rebuilds can resolve the same dependencies differently and the pinned wheels installed afterward may change behavior without a source change — should we pin pip to a tested version in the Dockerfile and bump it deliberately?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-guardrails-backend/Dockerfile.cpu around lines 29-31, the line that runs `pip
install -U "pip>=24.0"` makes the container build non-reproducible because it pulls the
latest pip that satisfies the floor each rebuild. Refactor this step to install a
specific, tested pip version (optionally via a build ARG with a default), so dependency
resolution behavior is stable across rebuilds. After pinning, keep the subsequent torch
CPU wheel install and requirements installation as-is, and ensure the pinned pip version
is documented for deliberate bumps.
There was a problem hiding this comment.
Skipping — the dependency set is already deterministic: torch==2.6.0 is pinned and requirements.txt is fully pinned, installed with --disable-pip-version-check. pip>=24.0 is just a floor for modern resolver/wheel support, so a floating pip patch doesn't change what gets installed. Leaving pip unpinned so the image keeps picking up pip security patches.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Thanks for the clarification — that makes sense. I’ll save this to memory once the PR is merged.
| COPY entrypoint.sh . | ||
| RUN chmod u+x entrypoint.sh | ||
|
|
||
| # Download the cache with the final user to ensure that files are in the right | ||
| # place and readable by the user | ||
| RUN mkdir /.cache/ /.local/ && chown -R 1001:1001 /opt/opik-guardrails-backend /.cache/ /.local/ | ||
| USER 1001:1001 |
There was a problem hiding this comment.
chmod u+x entrypoint.sh leaves the script executable only by root, so after USER 1001:1001 the CMD ["./entrypoint.sh"] startup hits permission denied — should we use COPY --chown=1001:1001 --chmod=755 entrypoint.sh . or chmod 755 before dropping privileges?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-guardrails-backend/Dockerfile.cpu around lines 33-39, the Dockerfile does
`COPY entrypoint.sh .` and then `RUN chmod u+x entrypoint.sh`, which leaves the file
executable only for root (mode 0744). Immediately after, it switches to `USER 1001:1001`
and starts with `CMD ["./entrypoint.sh"]`, so the non-root user cannot execute the
script. Update this hunk to ensure entrypoint.sh is owned by 1001:1001 and has
world-executable permissions before the USER directive — preferably via `COPY
--chown=1001:1001 --chmod=755 entrypoint.sh .`, or by adding `chown 1001:1001
entrypoint.sh && chmod 755 entrypoint.sh` in the RUN step.
There was a problem hiding this comment.
Fixed in b13f7f5 — changed chmod u+x to chmod 755 on entrypoint.sh. Since the file is mode 644 in git and COPY brings it in root-owned, u+x only granted execute to root; after USER 1001:1001 the non-root runtime user could hit permission denied on CMD ["./entrypoint.sh"]. 755 makes it executable for the runtime user.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Commit b13f7f5 addressed this comment by changing the script permissions to chmod 755, allowing user 1001 to execute it after the USER directive. Ownership remains unchanged, but root ownership does not prevent execution with mode 755.
| if [[ "$GUARDRAILS_ENABLED" == "true" ]]; then | ||
| cmd="$cmd --profile guardrails" | ||
| if [[ "$GUARDRAILS_MODE" == "cpu" ]]; then | ||
| cmd="$cmd --profile guardrails-cpu" | ||
| else | ||
| cmd="$cmd --profile guardrails" |
There was a problem hiding this comment.
Duplicated guardrails mode branching
The GUARDRAILS_MODE mapping is duplicated in set_containers_for_profile, get_verify_cmd, get_start_cmd, and get_docker_compose_cmd, so a future rename or third mode can drift out of sync — should we factor a small helper that returns the guardrails profile/flag/container set from one source of truth?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Commit f751f35 addressed this comment by introducing guardrails_profile(), guardrails_flag(), and guardrails_container() helpers as a single source of truth. The duplicated GUARDRAILS_MODE branching in set_containers_for_profile, get_verify_cmd, get_start_cmd, and get_docker_compose_cmd was replaced with calls to those helpers.
…nner support opik.sh: --guardrails now auto-detects an NVIDIA GPU via nvidia-smi and uses the GPU image when present, otherwise the CPU image. --guardrails-cpu still forces CPU. dev-runner.sh: add --guardrails and --guardrails-cpu modifier flags (previously no guardrails support). Detection is delegated to opik.sh; dev-runner forwards the flag to every opik.sh call and prints the OPIK_GUARDRAILS_URL_OVERRIDE for the directly-published :5000 endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| # Detect a usable NVIDIA GPU so --guardrails can pick the GPU vs CPU image. | ||
| # True only if nvidia-smi exists AND runs successfully (driver + GPU present). | ||
| guardrails_gpu_available() { | ||
| command -v nvidia-smi >/dev/null 2>&1 && nvidia-smi >/dev/null 2>&1 | ||
| } |
There was a problem hiding this comment.
GPU probe skips Docker runtime
guardrails_gpu_available() returns true whenever nvidia-smi exists, but the GPU service in deployment/docker-compose/docker-compose.yaml also needs the NVIDIA Container Toolkit/runtime, so --guardrails can pick guardrails on hosts that can't start the GPU profile and the launcher fails even though guardrails-cpu would work — should we gate on Docker runtime support too or fall back to CPU?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In opik.sh around
lines 703-707, the `guardrails_gpu_available()` helper currently returns true based
solely on `nvidia-smi` succeeding, which is insufficient because Docker may still lack
NVIDIA Container Toolkit/runtime support and the GPU guardrails container won’t start.
Refactor by extending the detection to also confirm Docker GPU support (for example,
check Docker for the NVIDIA runtime/toolkit or perform a lightweight `docker run --gpus
...`/compose smoke test) and only return true when both host tools and Docker GPU
support are present. Then in the guardrails flag logic that consumes this function
(around lines 722-730), keep the CPU fallback behavior when GPU mode cannot be started,
and ensure `--guardrails-cpu` still forces CPU regardless of detection.
There was a problem hiding this comment.
Commit 016c9ba addressed this comment by removing the NVIDIA runtime reservation from the guardrails Docker service and updating it to use a GPU-capable image that runs on CPU when no GPU is present. That means --guardrails no longer depends on Docker/NVIDIA Container Toolkit support to start, so the launch failure described in the comment is avoided.
| if guardrails_gpu_available; then | ||
| GUARDRAILS_MODE=gpu | ||
| echo "🖥️ NVIDIA GPU detected — using the GPU guardrails image" | ||
| else | ||
| GUARDRAILS_MODE=cpu |
There was a problem hiding this comment.
GPU auto-select can hard-fail
guardrails_gpu_available() only checks host nvidia-smi, so GUARDRAILS_MODE can be set to gpu even when Docker can't satisfy the NVIDIA runtime reservation and get_docker_compose_cmd() launches --profile guardrails on a machine that should use the CPU service — should we also gate on Docker's runtime support or fall back to --guardrails-cpu after docker compose up fails?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In opik.sh around
lines 724-730 (the guardrails GPU vs CPU mode selection right after
GUARDRAILS_ENABLED=true), fix the logic so it doesn’t set GUARDRAILS_MODE=gpu solely
based on host nvidia-smi. Refactor guardrails_gpu_available() (lines ~703-707) to also
verify Docker can actually run the NVIDIA workload (e.g., check Docker for the nvidia
runtime/GPU availability), or add a defensive fallback: if get_docker_compose_cmd()
launches the GPU guardrails profile and docker compose up fails, automatically switch
GUARDRAILS_MODE to cpu and retry with the CPU guardrails service/profile. Ensure the
final chosen mode is only GPU when the compose start is runnable; otherwise default to
--guardrails-cpu behavior.
There was a problem hiding this comment.
Commit 016c9ba addressed this comment by removing the NVIDIA device reservation from the guardrails service and documenting that the GPU-capable image now runs on CPU when no GPU is present. That eliminates the Docker runtime reservation hard-fail path the comment warned about, so --guardrails no longer depends on Docker having NVIDIA runtime support.
| if [ -n "$GUARDRAILS_OPIK_FLAG" ]; then | ||
| echo "" | ||
| echo -e "${BLUE}Guardrails enabled:${NC}" | ||
| echo " # The guardrails backend is published directly on port 5000." | ||
| echo " export OPIK_GUARDRAILS_URL_OVERRIDE='http://localhost:5000'" | ||
| fi |
There was a problem hiding this comment.
show_access_information() and show_restart_help() hardcode OPIK_GUARDRAILS_URL_OVERRIDE='http://localhost:5000' while compose uses ${OPIK_GUARDRAILS_PORT:-5000}, so overriding the port or running two guardrails-enabled worktrees points the SDK at the wrong address and causes Docker to fail at bind time — should we derive the override from OPIK_GUARDRAILS_PORT (exporting it per-worktree in worktree-utils.sh) and gate the hint to modes that actually publish the port?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
scripts/dev-runner.sh around lines 1253-1258: 1. Stop hardcoding `http://localhost:5000`
in both `show_access_information()` and `show_restart_help()`. Derive the effective port
from `OPIK_GUARDRAILS_PORT` (defaulting to 5000 when unset), e.g.
`http://localhost:${OPIK_GUARDRAILS_PORT:-5000}`, and use that value for the printed
`OPIK_GUARDRAILS_URL_OVERRIDE` export and any preflight/validation logic so Docker binds
and the SDK override stay in sync. 2. In `show_restart_help()`, gate the localhost
override hint to modes that actually publish the guardrails port on the host (e.g.
`--local-be` / `--local-be-fe`); for `--restart --guardrails` / `--restart
--guardrails-cpu`, either print the correct exposed URL or omit the hint to avoid
misleading users when port 5000 isn't reachable. 3. In scripts/worktree-utils.sh,
initialize and export `OPIK_GUARDRAILS_PORT` using the same per-worktree port allocation
logic as the other per-worktree ports, so multiple guardrails-enabled sessions bind
distinct host ports and the printed hint matches the actual compose mapping.
There was a problem hiding this comment.
Commit f751f35 addressed this comment by exporting OPIK_GUARDRAILS_PORT per worktree and using it when printing OPIK_GUARDRAILS_URL_OVERRIDE, so the hinted SDK URL now matches the compose port override. It also updates the guardrails access info to reference the derived port instead of hardcoding 5000.
…ement) The nvidia device reservation and nvidia-smi auto-detection changed how `./opik.sh --guardrails` behaves: it began requiring a GPU (device reservation) or switching to a CPU source-build (auto-detect), which broke the E2E CI flow that runs `./opik.sh --backend --port-mapping --guardrails` on GPU-less runners. Restore the original behavior: --guardrails runs the published GPU-capable image on CPU fallback (works everywhere, as before). --guardrails-cpu remains the explicit slim CPU-only image. dev-runner keeps both flags as simple forwards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| if [[ "$GUARDRAILS_ENABLED" == "true" ]]; then | ||
| cmd="$cmd --guardrails" | ||
| if [[ "$GUARDRAILS_MODE" == "cpu" ]]; then | ||
| cmd="$cmd --guardrails-cpu" | ||
| else | ||
| cmd="$cmd --guardrails" |
There was a problem hiding this comment.
Repeated guardrails mode switch
GUARDRAILS_MODE now maps to --guardrails-cpu vs --guardrails in three command builders, so any future flag or mode change has to stay in lockstep or the scripts drift — should we centralize that mapping in a tiny helper and reuse it from all three?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Commit f751f35 addressed this comment by centralizing the guardrails mode mapping into փոքր helper functions (guardrails_flag, guardrails_profile, and guardrails_container) and reusing them in the command/container builders. This removes the repeated GUARDRAILS_MODE branching across the three command builders the comment called out.
- alerts (BE): evaluate the guardrail-type filter per trigger with anyMatch,
so an unfiltered TRACE_GUARDRAILS_TRIGGERED trigger is no longer narrowed by a
sibling filtered trigger; fail open on unexpected payload shape
- alerts (FE): collect every filter:guardrail_type config (dedup via Set) instead
of reading only the first, matching the BE OR semantics on reopen/save
- metrics (test): assert the GUARDRAIL_NAME breakdown returns exactly {TOPIC, PII}
with deterministic fixture data instead of per-item membership
- migration 000090: forward-only rollback note (enum value cannot be reverted once
rows use it), consistent with 000033/000035
- opik.sh: centralize GUARDRAILS_MODE -> profile/flag/container mapping in helpers
- dev-runner.sh: derive the guardrails URL hint from OPIK_GUARDRAILS_PORT
- sdk: log guardrail results to the trace's project so project-scoped reads
(traces list) match them
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
🔴 size/XL
python
Review comments — resolutions (
|
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
The guardrails traces column and alert triggers only read already-logged guardrail data — they don't require the guardrails validation service to be deployed. Force GUARDRAILS_ENABLED on in the FE regardless of the backend feature-toggle config so the features are available in every deployment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alexkuzmik
added
test-environment
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
| const DEFAULT_STATE: FeatureToggles = { | ||
| [FeatureToggleKeys.PYTHON_EVALUATOR_ENABLED]: false, | ||
| [FeatureToggleKeys.GUARDRAILS_ENABLED]: false, | ||
| // Guardrails FE features (traces column, alert triggers) only read already-logged | ||
| // guardrail data — they don't depend on the guardrails validation service being | ||
| // deployed. Keep them always available regardless of backend config. | ||
| [FeatureToggleKeys.GUARDRAILS_ENABLED]: true, |
There was a problem hiding this comment.
Backend guardrail toggle is ignored
FeatureTogglesProvider forces FeatureToggleKeys.GUARDRAILS_ENABLED back to true after merging data, so each refresh rewrites the API response and the frontend never reflects a backend-disabled guardrails toggle. According to the PR description, guardrails are still opt-in, so can we keep the server value here and drive UI availability from the existing guardrails data instead?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/contexts/feature-toggles-provider.tsx around lines 20-25 in
`DEFAULT_STATE` and around lines 69-94 inside the `useEffect` in
`FeatureTogglesProvider`, stop hard-coding and re-forcing
`FeatureToggleKeys.GUARDRAILS_ENABLED` to `true`. Refactor so `setFeatures` merges
`{...DEFAULT_STATE, ...data}` without overwriting the backend’s value (if the backend
omits the key, rely on the declared default, not a forced true). Then update the
guardrails-related UI availability logic to be driven by the existing guardrails
data/availability (not by overriding this toggle client-side); search the codebase for
where guardrails data availability is already determined and use that to gate the
guardrails UI/columns/triggers.
There was a problem hiding this comment.
Intentional — same rationale as the line-25 thread: the FE guardrail features only read already-logged data and don't depend on the validation service, so GUARDRAILS_ENABLED is kept on in the FE regardless of the backend toggle. Documented inline in DEFAULT_STATE.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Thanks for the clarification — that makes sense. I’ll save this to memory once the PR is merged.
| setFeatures({ | ||
| ...DEFAULT_STATE, | ||
| ...data, | ||
| // Force-on regardless of what the backend reports — see DEFAULT_STATE. | ||
| [FeatureToggleKeys.GUARDRAILS_ENABLED]: true, |
There was a problem hiding this comment.
FeatureToggleKeys.GUARDRAILS_ENABLED is force-set to true after every backend fetch, so server-side false values are discarded and LogsPage/TracesPage always render the "Set a guardrail" button even for users without configure permissions — should we let the backend value be authoritative and gate those entry points with canConfigureWorkspaceSettings, as .agents/skills/opik-frontend/permissions.md suggests?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-frontend/src/contexts/feature-toggles-provider.tsx around lines 76-80, remove
the unconditional force-on of FeatureToggleKeys.GUARDRAILS_ENABLED inside the useEffect
that calls setFeatures. Instead, set features by merging DEFAULT_STATE with `data` so
the backend value for GUARDRAILS_ENABLED is authoritative, falling back to DEFAULT_STATE
only when the backend omits the field. If guardrails must always be on for a
UI-dependency reason, implement that as a separate, clearly named frontend-only flag
rather than overwriting the backend toggle. Additionally, update the v2 LogsPage and v1
TracesPage entry points to check canConfigureWorkspaceSettings (via usePermissions)
before rendering the "Set a guardrail" button/dialog, with a sensible read-only fallback
when the user lacks permission. Follow the same guarded permission check pattern used by
other configure surfaces (e.g., SignalsPage diagnostics gating), as permissions.md
requires.
There was a problem hiding this comment.
Good suggestion — deferring as a follow-up. Gating the "Set a guardrail" entry points behind canConfigureWorkspaceSettings is a reasonable permission refinement but is orthogonal to this PR (CPU image + dashboard breakdown + alert type filter). Tracking separately.
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Thanks for the clarification — noted that this permission refinement is reasonable but out of scope for this PR. I'll save this to memory once the PR is merged.
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
thiagohora
left a comment
thiagohora
left a comment
There was a problem hiding this comment.
Solid, well-tested, and idiomatic across backend, FE, Docker, scripts, and the SDK. No blocking issues found.
Verified during review:
- Guardrails alert payload reaching
matchesGuardrailTypeFilteris the in-memoryList<Guardrail>(Guava@Subscribe, synchronous), soinstanceofholds; payload contains only failed guardrails, sofailedGuardrailTypesis accurately named. - The breakdown query joins
guardrails g, filtersresult='failed', groups byg.name(aStringtype name) — matches the test assertions;count(DISTINCT g.id)handles ReplacingMergeTree duplicates, consistent with the existing non-breakdown query. - Migration
000090re-lists all existing enum values + the new one (no truncation). opik.shmatches--guardrails-cpubefore--guardrails(substring hazard handled).- SDK
current_trace.project_name or self._client._project_nameis a genuine project-scoping fix.
Left three inline notes (all minor): the cross-trigger OR edge case in the alert evaluation, a trailing-newline nit on the migration, and a heads-up on the now-unconditional GUARDRAILS_ENABLED toggle. None are merge blockers.
| return true; | ||
| } | ||
|
|
||
| return guardrailTriggers.stream().anyMatch(trigger -> { |
There was a problem hiding this comment.
Cross-trigger OR (edge case): the type filter is OR-ed across all TRACE_GUARDRAILS_TRIGGERED triggers here, while isWithinProjectScope separately OR-s project IDs across the same triggers. If an alert ever has two guardrails triggers scoped to different projects with different type filters (e.g. project A→PII, project B→TOPIC), a TOPIC failure in project A would pass both checks and fire, even though A's trigger only wanted PII.
This is unreachable if the UI allows only one guardrails trigger per alert, and it mirrors the existing project-scope design — so low severity. Worth a short comment noting the assumption, or a guard, if multi-trigger guardrails alerts are possible.
There was a problem hiding this comment.
Acknowledged — real but low-probability edge case. The type filter and project scope are OR-ed independently across all TRACE_GUARDRAILS_TRIGGERED triggers, so an alert hand-configured with two guardrails triggers scoped to different projects and different type filters could pass both checks and fire. The UI builds a single guardrails trigger per alert in the common path, so this requires a crafted multi-trigger config; treating it as a known edge case (not a blocker) and tracking a follow-up if per-trigger project×type correlation is needed.
🤖 Reply posted via /address-github-pr-comments
| -- Forward-only (matches 000033/000035): reverting is unsafe once any alert_trigger_configs row | ||
| -- uses 'filter:guardrail_type', since MODIFY-ing the enum back would truncate those values. Recovery | ||
| -- means restoring the previous enum only after deleting/migrating such rows. | ||
| --rollback empty |
There was a problem hiding this comment.
Nit: per the migration-script convention, files should end with a trailing blank line (see 000035). This one ends on --rollback empty. The enum MODIFY itself correctly re-lists all existing values plus the new one — no truncation risk. ✅
There was a problem hiding this comment.
Fixed in b13f7f5 — the migration now ends with a trailing blank line, matching the convention in 000035. (The enum MODIFY already re-listed all values, as you noted.)
🤖 Reply posted via /address-github-pr-comments
There was a problem hiding this comment.
Commit b13f7f5 addressed this comment by adding a trailing blank line after --rollback empty.
| // Guardrails FE features (traces column, alert triggers) only read already-logged | ||
| // guardrail data — they don't depend on the guardrails validation service being | ||
| // deployed. Keep them always available regardless of backend config. | ||
| [FeatureToggleKeys.GUARDRAILS_ENABLED]: true, |
There was a problem hiding this comment.
Behavior flag (not a bug): combined with the force-on in the setFeatures merge below, GUARDRAILS_ENABLED is now unconditionally true and the backend can no longer turn it off. Intentional per the PR description, but it makes this toggle effectively dead — worth confirming no deployment relied on hiding these FE surfaces.
There was a problem hiding this comment.
Confirmed intentional. The FE guardrail surfaces (traces column, alert triggers) only read already-logged guardrail data — they don't require the guardrails validation service to be deployed — so they're kept available regardless of the backend toggle. Checked that no deployment relies on the backend flag to hide these surfaces. Rationale documented inline in DEFAULT_STATE.
🤖 Reply posted via /address-github-pr-comments
…ailing newline Address PR #7506 review: - Dockerfile.cpu: chmod 755 (not u+x) so entrypoint.sh stays executable for USER 1001:1001 (it is mode 644 in git); u+x only granted execute to the root owner. - migration 000090: end file with a trailing blank line per the convention in 000035. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Details
Batch 1 of guardrails improvements: (1) a slim, multi-arch CPU-only guardrails image (
Dockerfile.cpu) plus a--guardrails-cpuflag inopik.shandscripts/dev-runner.sh, so guardrails builds/runs on CPU-only and arm64 machines; (2) aGuardrail namebreakdown on the existing "Failed guardrails" dashboard metric (one series per guardrail type, e.g. PII / Topic); (3) a guardrail-type filter on the guardrails alert so an alert can target specific guardrail types.--guardrailsruns the published GPU-capable image on CPU fallback (unchanged),--guardrails-cpuuses the slim CPU image.GUARDRAILS_FAILED_COUNTmetric andtrace:guardrails_triggeredalert.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
ProjectMetricsWithBreakdownResourceTest(group-by-guardrail-name) andAlertResourceTest(type-filtered alert fires only for the matching type) pass.npm run typecheck+ eslint clean;helpers.test.tscovers the alert form↔payload mapping.bash -n opik.shandbash -n scripts/dev-runner.shpass; verified guardrails stay out of the default compose profile (docker compose config --services).Documentation
🤖 Generated with Claude Code