feat(evaluators): add new lluna client#213
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
namrataghadi-galileo
changed the title
| def luna_config() -> dict[str, Any]: | ||
| """Build the direct Luna evaluator config used by the composite control.""" | ||
| config: dict[str, Any] = { | ||
| "scorer_label": LUNA_SCORER_LABEL, | ||
| "threshold": LUNA_THRESHOLD, | ||
| "operator": "gte", | ||
| "payload_field": "output", | ||
| "on_error": "allow", | ||
| } | ||
| if GALILEO_PROJECT_ID: | ||
| config["project_id"] = GALILEO_PROJECT_ID | ||
| return config |
There was a problem hiding this comment.
payload_field, on_error, and project_id are set at the top level of the evaluator config, but LunaEvaluatorConfig declares none of them and the shared BaseModel uses extra="ignore", so all three are silently dropped. The example demonstrates configuration that has no effect — users following it will believe these settings are honored. Either implement these as first-class fields on LunaEvaluatorConfig (looks intended for payload_field and on_error), or remove them from the example and README.
| "message": self._truncated_message(result.message), | ||
| } | ||
| metadata = dict(result.metadata or {}) | ||
| metadata["selected_data"] = data |
There was a problem hiding this comment.
This unconditionally injects the raw selector output (prompts, tool args, model output, potentially PII or secrets) into evaluator metadata for every leaf control. The metadata flows out through EvaluationResponse.matches/errors/non_matches to API clients, and is also fanned out into OTEL span attributes by the SDK's otel_sink (every metadata key becomes agent_control.metadata.<key>). The server's _sanitize_control_match only redacts internal evaluator error strings — it doesn't touch metadata. No size cap, redaction, or opt-out. Gate selected_data behind a config flag with truncation, or extend the sanitizer to scrub it before exporting.
| dependencies = [ | ||
| "agent-control-sdk", | ||
| "agent-control-evaluator-galileo", | ||
| ] |
There was a problem hiding this comment.
dependencies declares only agent-control-sdk and agent-control-evaluator-galileo, but [tool.uv.sources] lists six workspace packages. [tool.uv.sources] only overrides sources for already-declared dependencies — it does not add packages. The SDK imports agent_control_telemetry (and other workspace packages) at runtime, but those are not vendored in editable mode, so demo_agent.py's first import will fail when a user runs uv run python setup_controls.py as documented. See examples/google_adk_plugin/pyproject.toml for the correct pattern (all workspace packages listed under both dependencies and [tool.uv.sources]).
There was a problem hiding this comment.
Still seeing this: uv run from examples/galileo_luna does not install the workspace packages listed only under [tool.uv.sources], so imports fail on agent_control_telemetry. Please add those workspace packages to dependencies as well.
| If the scorer requires explicit project resolution, set: | ||
|
|
||
| ```bash | ||
| export GALILEO_PROJECT_ID="00000000-0000-0000-0000-000000000000" | ||
| ``` |
There was a problem hiding this comment.
The README tells users to set GALILEO_PROJECT_ID for project-scoped scorer resolution, but LunaEvaluatorConfig has no project_id field and setup_controls.py places it at the top level where it gets dropped by extra="ignore". The instruction is misleading. Either implement the field end-to-end through the config and scorer payload, or remove this section.
| metadata={ | ||
| "error": error_detail, | ||
| "error_type": type(error).__name__, | ||
| "scorer_label": self.config.scorer_label, | ||
| "scorer_id": self.config.scorer_id, | ||
| "scorer_version_id": self.config.scorer_version_id, | ||
| }, |
There was a problem hiding this comment.
Storing error_detail in metadata["error"] lets raw upstream error text bypass the server sanitizer. _sanitize_control_match redacts result.error and metadata["condition_trace"]["error"], but not top-level metadata["error"], so HTTP error bodies, exception messages with URLs, and internal hints leak through to API responses and telemetry. error_type is already in metadata, which is the part callers can safely act on — error_detail is redundant. Drop it, or replace with a safe error code.
| def _get_client(self) -> GalileoLunaClient: | ||
| """Get or create the Galileo Luna client.""" | ||
| if self._client is None: | ||
| self._client = GalileoLunaClient() | ||
| return self._client |
There was a problem hiding this comment.
Evaluator instances are LRU-cached and reused across calls; the base Evaluator docstring explicitly warns against storing mutable request-scoped state on self. _get_client mutates self._client with no synchronization. There is no asyncio race within _get_client itself (no await), but multi-thread or multi-event-loop reuse can construct two clients on the first concurrent call, orphan one of them, and leak its httpx.AsyncClient connection pool. Auth already validates in __init__ — construct the client eagerly there too.
| metadata = dict(result.metadata or {}) | ||
| metadata["selected_data"] = data | ||
| metadata["condition_trace"] = trace |
There was a problem hiding this comment.
metadata = dict(result.metadata or {}) then metadata["selected_data"] = data unconditionally overwrites whatever the evaluator deliberately put under selected_data. Evaluators do return that key (see the mock in engine/tests/test_core.py:160), and an evaluator might want to ship a sanitized or transformed version. The contract is not currently clear about who owns the key — consider namespacing the engine-injected value (e.g. engine_selected_data) so it cannot collide with evaluator-supplied metadata.
| headers: dict[str, str] | None, | ||
| ) -> tuple[str, dict[str, str]]: | ||
| request_headers = dict(headers or {}) | ||
| if self.api_secret is None: |
There was a problem hiding this comment.
The endpoint/auth mode is selected implicitly from whichever credential is present: if an API secret is set, the client uses the internal route; otherwise it uses the public API-key route. If both credentials are present, the secret path wins silently. Please make this an explicit runtime/client mode, validate that the matching credential is present, and log the selected mode without logging secrets.
|
|
||
| text = _coerce_payload_text(data) | ||
| scorer_label = self.config.scorer_label or "" | ||
| if "output" in scorer_label: |
There was a problem hiding this comment.
For scalar selected data, input vs output routing is inferred by checking whether the scorer label contains output. That is surprising behavior for a public evaluator contract, especially because the example config suggests there is an explicit payload_field knob. Please make the payload side explicit in config and test it, or document this heuristic very clearly.
| if isinstance(score, list): | ||
| return threshold in score | ||
| if isinstance(score, dict): | ||
| if isinstance(threshold, str) and threshold in score: |
There was a problem hiding this comment.
For dict scores, contains currently matches both keys and values. That can trigger unexpectedly when the threshold appears as a field name rather than as the scorer value. Please either narrow this to values-only or document and test the intended semantics explicitly.
| for preview_key in ("engine_selected_data_preview", "selected_data_preview"): | ||
| preview = metadata.get(preview_key) | ||
| if isinstance(preview, dict) and "value" in preview: | ||
| safe_metadata["input"] = preview["value"] |
There was a problem hiding this comment.
This is a silent regression in the privacy posture of event metadata. _safe_event_metadata deliberately strips the four *_data* keys to keep selected-data out of observability sinks, but the line below copies preview["value"] into metadata["input"], which is not in _DEBUG_METADATA_ATTRIBUTE_KEYS. The OTEL sink (otel_sink.py) re-exports every metadata key as agent_control.metadata.<key>, so the same preview that was just filtered out leaks back through under a new name.
The preview's own redaction is shallow:
- Selector paths that resolve to a primitive (e.g.
input.api_key) return a plain string._truncate_stringonly truncates, it never redacts, so a 32-char secret lands verbatim on the span. - Dict redaction is key-name substring matching against a 7-entry list; nested sensitive strings (e.g.
{"prompt": "my password is hunter2"}) pass through.
Suggestions:
- Drop the synthesized
inputfield entirely, or - Make it explicit opt-in via an env flag (mirroring
AGENT_CONTROL_INCLUDE_RAW_SELECTED_DATA), or - Use a namespaced key like
engine_input_previewand add it to the OTEL strip set.
There was a problem hiding this comment.
We need to keep this behavior for now because the UI’s control-span Messages/Input view depends on the synthesized metadata["input"] field. The control span search result exposes input as a first-class display field, and in the current OTEL ingestion path this value is derived from the exported control event metadata. Removing the alias would preserve the stricter privacy posture, but it would also regress the Messages view by no longer showing the selected input text for control spans.
We can wait for the users to come back on this to us. All other spans show raw data in the Messages View in UI
| def _has_text(value: str | None) -> bool: | ||
| return value is not None and value != "" |
There was a problem hiding this comment.
_has_text only rejects "", but the client's _has_value strips whitespace before testing. So " " bypasses the "No data to score with Luna" shortcut, reaches client.invoke, and raises ValueError("At least one of input or output must be provided."). The outer except records that as an evaluator error.
For deny controls, engine.process() flips is_safe=False when a deny control errors, so whitespace data fails closed, the opposite of the explicit no-data path. Align with the client's semantics:
def _has_text(value: str | None) -> bool:
return value is not None and value.strip() != ""The existing test_evaluator_does_not_call_api_for_empty_data only covers ""; add a " " case.
There was a problem hiding this comment.
Changed _has_text to use value.strip() != "", matching the client’s _has_value semantics. I also parameterized the existing empty-data test in test_luna_evaluator.py (line 443) so both "" and " " take the no-data path and do not call the API.
| def _derive_api_url(self, console_url: str) -> str: | ||
| """Derive the API URL from a Galileo Console URL.""" | ||
| url = console_url.rstrip("/") | ||
|
|
||
| if "console." in url: | ||
| return url.replace("console.", "api.") | ||
| if "console-" in url: | ||
| return url.replace("console-", "api-", 1) | ||
|
|
||
| if url.startswith("https://"): | ||
| return url.replace("https://", "https://api.") | ||
| if url.startswith("http://"): | ||
| return url.replace("http://", "http://api.") | ||
|
|
||
| return url |
There was a problem hiding this comment.
The two in checks operate on the entire URL, not on the hostname prefix. A few cases this gets wrong:
https://my-console.example.combecomeshttps://my-api.example.cominstead of being left alone (or being something sensible likehttps://api.my-console.example.com).https://app.galileo.ai/console.htmlrewrites the path: theconsole.in/console.htmltriggers the substitution and yieldshttps://app.galileo.ai/api.html. The host isn't even touched.
The companion tests in tests/test_luna_coverage_gaps.py only exercise canonical Galileo hostnames (console.galileo.ai, console-staging.galileo.ai) and a couple of plain example.com hosts, so the bug surface is invisible to CI.
Fix:
from urllib.parse import urlsplit, urlunsplit
parts = urlsplit(url)
host = parts.hostname or ""
if host.startswith("console."):
new_host = "api." + host[len("console."):]
elif host.startswith("console-"):
new_host = "api-" + host[len("console-"):]
else:
... # decide on fallback policy explicitlyOperate on the hostname only, and gate substitution on startswith.
Separate but related: the fallback at the bottom (https://example.com -> https://api.example.com) silently rewrites the host of any non-console URL. The plain-host tests codify it as intended, but it is surprising behavior for users supplying an explicit GALILEO_CONSOLE_URL to their own infrastructure. Worth confirming this is intentional and documenting it.
There was a problem hiding this comment.
Implemented the URL derivation fix in client.py (line 255). It now parses with urlsplit, rewrites only hostname prefixes console. and console-, and preserves the existing fallback of prefixing non-console HTTP(S) hosts with api.. I added docstring text to make that fallback explicit.
| def _handle_error( | ||
| self, | ||
| error: Exception, | ||
| ) -> EvaluatorResult: | ||
| error_detail = str(error) | ||
| return EvaluatorResult( | ||
| matched=False, | ||
| confidence=0.0, | ||
| message=f"Luna evaluation error: {error_detail}", | ||
| metadata={ | ||
| "error_type": type(error).__name__, | ||
| "scorer_label": self.config.scorer_label, | ||
| "scorer_id": self.config.scorer_id, | ||
| "scorer_version_id": self.config.scorer_version_id, | ||
| }, | ||
| error=error_detail, | ||
| ) |
There was a problem hiding this comment.
On any exception, _handle_error returns matched=False, confidence=0.0. luna2 already exposes on_error: 'allow' | 'deny' for safety-critical flows, so users mixing both evaluators get inconsistent failure semantics.
Partial mitigation today: for deny controls, engine.process() sets is_safe=False when a deny control errors (see engine/core.py around line 765), so the safety-critical path is already fail-closed. The gap is real for steer/observe actions and for parity with luna2.
Consider adding:
on_error: Literal["allow", "deny"] = "allow"to LunaEvaluatorConfig and honoring it in _handle_error. Note that EvaluatorResult forbids matched=True when error is set, so the deny variant should still produce matched=False but with metadata the runner interprets as fail-closed (same contract as luna2).
There was a problem hiding this comment.
I don’t think we should add on_error to Luna in this patch.
The current engine contract treats evaluator failures through EvaluatorResult.error: when error is set, matched must be False, and deny controls already fail closed at the engine level when a deny evaluation errors. For steer/observe, adding on_error="deny" only as metadata would not change behavior today because the engine does not interpret fallback_action metadata. To make it behaviorally effective, Luna would either need to suppress error and synthesize a matched result, which loses the evaluator-health signal, or we would need a shared engine/model contract for “error fallback action.”
2 participants
Summary
Scope
Risk and Rollout
Testing
make check(or explained why not)Checklist