feat(evals): agent eval harness + finding_enricher reference implementation (ADR-0050)#265
Conversation
…R-0050) First agent wired under the generic eval system (ADR-0050 rollout: highest- risk first). Thin layer over pydantic-evals: - cliff/evals/: adapter (drives any workspace agent through one call), registry (AgentEvalSpec — supported assertions + budget + model), cases (typed JSONL loader), evaluators (deterministic; "code first, judge last"). - The citation gate is SPLIT (data-driven, the live run taught us this): structural fabrication (fake GHSA / garbled SHA / non-http) is the zero-tolerance HARD gate (reuses the production reference_verifier); a plausible URL that 404s is a GRADED dead-link metric, not a hard fail. - Two lanes (ADR-0050 §5): test_evals_harness.py (CI, deterministic, FunctionModel/TestModel + injected http — proves the scorer); test_evals_finding_enricher.py (live, key-gated via conftest — runs the real enricher over eval/finding_enricher.jsonl: hard gates zero-tolerance, graded metrics on a floor). - Dataset: 6 cases covering well-known CVE, dependency CVE, scanner-jargon title, two no-CVE abstention cases, and a fabrication-bait case. Verified: CI lane 9/9 + 104 deterministic agent tests green; the live lane passes against real gpt-4o-mini (no structural fabrication, abstention holds, graded metrics clear the floor). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 19 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR introduces the Cliff agent-evaluation harness (ADR-0050): spec registry, JSONL case loader, deterministic evaluators for citations/CVEs/CVSS/title/abstention, an adapter to run agents under eval conditions, a runner to aggregate graded and hard-fail metrics, and CI/live tests with sample fixtures. ChangesAgent Evaluation Harness (ADR-0050)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| edge_case: str | None = None | ||
| abstain: bool = False | ||
| finding: dict[str, Any] | ||
| expected: dict[str, Any] = Field(default_factory=dict) |
There was a problem hiding this comment.
expected is still a free-form dict[str, Any], so malformed JSONL rows can reach check_cve_ids()/check_cvss_within() and sometimes pass silently; should we replace it with a typed submodel or validators so bad rows fail in load_cases()?
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
backend/cliff/evals/cases.py around lines 23-53, refactor the EvalCase model so the
expected field is no longer a free-form dict[str, Any]. Replace expected: dict[str, Any]
(line 33) with a typed Pydantic submodel (or add field validators) that explicitly
validates the evaluator contract (e.g., expected.cve_ids must be a list of strings, and
expected.cvss must be numeric and within any required bounds), so malformed JSONL rows
fail during EvalCase.model_validate_json. Ensure load_cases (lines 36-52) still surfaces
the same {path.name}:{line_no} context on validation errors, but now the errors are
raised for bad expectation shapes/types instead of leaking into
check_cve_ids()/check_cvss_within().
There was a problem hiding this comment.
Commit af806fc addressed this comment by replacing expected with a typed Expected Pydantic submodel. The new model forbids extra keys and constrains the expectation fields, so malformed JSONL rows now fail during load_cases() instead of flowing through to check_cve_ids() / check_cvss_within().
| refs = getattr(output, "references", None) | ||
| check = ( | ||
| await clean_references(refs, http=http) | ||
| if http is not None | ||
| else await clean_references(refs) | ||
| ) | ||
| structural = [(u, r) for u, r in check.dropped if not r.startswith("http_")] |
There was a problem hiding this comment.
if http is not None is redundant since both branches call clean_references(refs, http=http); should we drop the branch and just check = await clean_references(refs, http=http)?
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
backend/cliff/evals/evaluators.py around lines 63-69 inside the async function
assess_references, remove the redundant if/else that checks `http is not None`. Refactor
it so you always call `clean_references(refs, http=http)` (letting http be None), since
clean_references already defaults `http` to None. Ensure the returned
`ReferenceAssessment` still uses the same `check.dropped` and `check.kept` logic.
There was a problem hiding this comment.
Commit af806fc addressed this comment by removing the if http is not None branch and always calling clean_references(refs, http=http). The returned ReferenceAssessment still uses the same check.dropped and check.kept values afterward.
| _LIVE_LLM_FILES = ( | ||
| "test_normalizer_agent", | ||
| "test_plain_description_eval", | ||
| "test_evals_finding_enricher", | ||
| ) |
There was a problem hiding this comment.
test_evals_finding_enricher is treated as live-LLM, but _api_key_set still only checks OPENAI_API_KEY/ANTHROPIC_API_KEY, so setups like OPENROUTER_API_KEY + CLIFF_EVAL_MODEL=openrouter/... get skipped; should we widen the gate to the same provider set as build_model() or derive it from LLM_ENV/LLM_MODEL?
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
`backend/tests/agents/conftest.py` around lines 25-29, `_LIVE_LLM_FILES` now includes
`test_evals_finding_enricher`, but the live-LLM gating logic via `_api_key_set` still
only recognizes `OPENAI_API_KEY` and `ANTHROPIC_API_KEY`, so tests are incorrectly
skipped for other `build_model()`-supported providers (e.g., `OPENROUTER_API_KEY`,
`GEMINI_API_KEY`, `OLLAMA_BASE_URL` with a model like `openrouter/...`). Refactor by
updating `_api_key_set` (or the surrounding skip condition in this file) to use the same
provider set as `build_model()`—either by checking all relevant provider env vars or
by inferring the needed gate from `CLIFF_EVAL_MODEL`/`LLM_ENV`/`LLM_MODEL`. Ensure that
when the environment is runnable with a non-OpenAI/Anthropic config,
`test_evals_finding_enricher` is no longer skipped.
There was a problem hiding this comment.
Commit af806fc addressed this comment by broadening _api_key_set to recognize additional LLM provider env vars (OPENROUTER_API_KEY, GEMINI_API_KEY, OLLAMA_BASE_URL) instead of only OpenAI/Anthropic. That lets test_evals_finding_enricher run in non-OpenAI/Anthropic live-LLM setups that were previously skipped.
| async def run_agent( | ||
| spec: AgentEvalSpec, | ||
| finding: dict[str, Any], | ||
| *, | ||
| env: dict[str, str] | None = None, | ||
| model_id: str | None = None, | ||
| model: Model | None = None, | ||
| prior_context: dict[str, dict[str, Any]] | None = None, | ||
| ) -> Any: | ||
| """Run *spec*'s agent over a single eval case and return its output object. | ||
|
|
||
| Provide ``model`` directly (CI lane: a ``FunctionModel``/``TestModel``) or |
There was a problem hiding this comment.
run_agent duplicates the agent setup/run plumbing from run_no_tools_agent, should we extract a shared helper so prompt/deps/agent.run changes stay in sync?
Want Baz to fix this for you? Activate Fixer
There was a problem hiding this comment.
Commit 02411fa addressed this comment by extracting the shared setup/run plumbing into _run, so model construction, WorkspaceDeps, prompt rendering, and agent.run live in one place. run_agent now delegates to that helper, which keeps future prompt/deps/run changes in sync.
| deps = WorkspaceDeps( | ||
| workspace_id="eval", | ||
| workspace_dir="/tmp/cliff-eval", | ||
| finding=finding, |
There was a problem hiding this comment.
run_agent hardcodes WorkspaceDeps(workspace_id="eval", workspace_dir="/tmp/cliff-eval") instead of going through the workspace layer under data/workspaces/ — should we route eval runs there so file-backed state stays isolated per workspace?
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
backend/cliff/evals/adapter.py around lines 42-45 inside run_agent, the WorkspaceDeps is
constructed with workspace_id="eval" and workspace_dir="/tmp/cliff-eval" which violates
CLAUDE.md’s workspace isolation rule and can cause file-backed agent/tool state to
bleed across eval cases. Refactor run_agent to allocate a unique workspace via the
workspace layer (under data/workspaces/<id>/) for each eval run/case—either by calling
the existing workspace creator from backend/cliff/workspace/ or by introducing a small
helper that returns an isolated workspace_id/workspace_dir. Then pass the allocated
workspace_id/workspace_dir into WorkspaceDeps so each case runs in its own directory.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cliff/evals/adapter.py`:
- Around line 40-41: The code currently sets resolved_model = model if model is
not None else build_model(env or {}, model_id) and thus ignores
spec.default_model; update the resolution logic so that when model is None you
prefer spec.default_model if present (or fall back to build_model with
model_id), e.g. check spec.default_model before calling build_model; modify the
resolved_model assignment in adapter.py (the resolved_model, build_model,
model_id and spec objects) to: if model is not None use model, else if
spec.default_model is not None use spec.default_model, else call build_model(env
or {}, model_id).
- Around line 24-32: The run_agent entrypoint uses broad Any types; replace raw
dict/Any usage with explicit Pydantic models and strict typing: create a
Pydantic model (e.g., AgentFinding) for the finding parameter and a model (e.g.,
AgentResult) for the return value, update the run_agent signature to accept
AgentEvalSpec, finding: AgentFinding | dict and return AgentResult (or
AgentResult | dict for transitional compatibility), and change
prior_context/type hints to a typed mapping or Pydantic model instead of
dict[str, dict[str, Any]]; inside run_agent, parse incoming dicts with
AgentFinding.parse_obj/AgentResult.parse_obj to validate, update callers to
pass/consume the models (or rely on the parse logic), and add/adjust tests to
assert model validation and typed outputs.
- Around line 49-50: The adapter currently returns the raw result.output from
agent.run (called with build_user_prompt(deps) and deps=deps) without checking
it against spec.output_type; update the code in adapter.py to validate
result.output against deps.spec.output_type (or spec.output_type available on
deps) after agent.run completes, converting/coercing if possible and raising a
clear error (or returning a standardized ValidationError object) when the type
mismatches; ensure this validation is applied before returning from the function
so evaluators never receive unvalidated objects.
In `@backend/cliff/evals/cases.py`:
- Around line 36-51: The load_cases function accepts tier: str | None but should
be narrowed to the same literal domain as EvalCase.tier to prevent silent empty
filters; change the tier parameter type on load_cases to the Literal union used
by EvalCase.tier (or reference the EvalCase.tier type/alias if one exists) so
the signature becomes e.g. tier: Literal["ci","live"] | None (or the EvalCase
tier alias) and keep the existing filtering logic (cases = [c for c in cases if
c.tier == tier]) unchanged.
In `@backend/cliff/evals/evaluators.py`:
- Line 31: The current regex
re.compile(r"\b[a-z0-9_-]+\.[a-z0-9_-]+\.[a-z0-9_.-]+\b") is too broad and
matches semantic versions like "2.17.1"; update that pattern in evaluators.py to
exclude purely numeric dotted sequences by adding a negative lookahead that
rejects all-numeric dot-separated tokens (i.e., ensure at least one segment
contains a letter or underscore), so scanner jargon detection no longer flags
semantic versions while still matching true dotted rule paths.
In `@backend/cliff/evals/registry.py`:
- Around line 27-47: Convert the dataclasses BudgetSpec and AgentEvalSpec into
Pydantic models: replace `@dataclass` with subclasses of pydantic.BaseModel, use
explicit typing (Optional[float], Optional[int], Optional[str],
Callable[[Model], Agent], Type[BaseModel], FrozenSet[str]) for fields like
max_usd, max_tokens, max_duration_s, build_agent, output_type,
supported_assertions, budget, default_model, live_only, eval_frozen; set
immutable behavior via Config (allow_mutation = False) or frozen=True, and use
pydantic.Field to provide defaults (None/False) where needed so
validation/serialization matches the rest of the backend schema layer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3fdfa9c6-32ac-4b1b-91e4-90f745656c45
📒 Files selected for processing (9)
backend/cliff/evals/__init__.pybackend/cliff/evals/adapter.pybackend/cliff/evals/cases.pybackend/cliff/evals/evaluators.pybackend/cliff/evals/registry.pybackend/tests/agents/conftest.pybackend/tests/agents/eval/finding_enricher.jsonlbackend/tests/agents/test_evals_finding_enricher.pybackend/tests/agents/test_evals_harness.py
Prep for the ADR-0050 hybrid (harness public, data private): - load_cases reads CLIFF_EVAL_DATASET_DIR (default = the public synthetic sample under tests/agents/eval/); the private cliff-os/eval project overrides it to point at the real/confidential golden sets, which never enter this public repo. - Extract the enricher run loop into cliff/evals/runners.run_enricher_eval (+ EvalRunResult) so both the in-repo live test AND the private cliff-os runner drive the same scorer over their own data. - README in tests/agents/eval/ documents the public-sample/private-data split. Verified: CI harness 9/9; live eval passes against gpt-4o-mini on the default sample AND with CLIFF_EVAL_DATASET_DIR pointed at an override dir (the bridge). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The agent-eval hybrid (ADR-0050) needs the private cliff-os/eval project to depend on the cliff backend to import the real agents. That requires cliff to be a buildable/installable package — it had no [build-system]. Add hatchling targeting the cliff package. No change to the in-place run flow (uv sync / uv run unchanged); `uv build` now produces cliff-<ver>-py3-none-any.whl. CI's install-smoke matrix validates the packaging on this PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
backend/cliff/evals/cases.py (1)
46-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow
tierparameter type to matchEvalCase.tierdomain.The
tierparameter acceptsstr | None, whileEvalCase.tierat Line 39 is constrained toLiteral["ci", "live"]. This type mismatch allows invalid tier values (e.g.,tier="foo") to pass type checking but fail silently at Line 61's filter, returning an empty list.As per coding guidelines, backend/cliff code should use strict type hints.
🔧 Proposed fix
-def load_cases(agent: str, *, tier: str | None = None) -> list[EvalCase]: +def load_cases(agent: str, *, tier: Literal["ci", "live"] | None = None) -> list[EvalCase]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cliff/evals/cases.py` at line 46, The load_cases function accepts tier: str | None which is too loose compared to EvalCase.tier (Literal["ci", "live"]); change the parameter type to match EvalCase.tier (e.g., Optional[Literal["ci", "live"]]) and update any imports as needed so callers and static checkers only allow "ci" or "live"; locate the load_cases function and the filter that checks case.tier and replace the str|None annotation with the Literal-based type so invalid tiers cannot be passed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@backend/cliff/evals/cases.py`:
- Line 46: The load_cases function accepts tier: str | None which is too loose
compared to EvalCase.tier (Literal["ci", "live"]); change the parameter type to
match EvalCase.tier (e.g., Optional[Literal["ci", "live"]]) and update any
imports as needed so callers and static checkers only allow "ci" or "live";
locate the load_cases function and the filter that checks case.tier and replace
the str|None annotation with the Literal-based type so invalid tiers cannot be
passed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f6fbe8d-2101-49c2-91e5-a93d179cd99c
📒 Files selected for processing (5)
backend/cliff/evals/__init__.pybackend/cliff/evals/cases.pybackend/cliff/evals/runners.pybackend/tests/agents/eval/README.mdbackend/tests/agents/test_evals_finding_enricher.py
| override = os.environ.get("CLIFF_EVAL_DATASET_DIR") | ||
| return Path(override) if override else _SAMPLE_DIR |
There was a problem hiding this comment.
CLIFF_EVAL_DATASET_DIR is used verbatim, so relative overrides can resolve against the job cwd and miss load_cases(); should we reject non-absolute values or anchor them to a known base path?
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
backend/cliff/evals/cases.py around lines 24-30, in the dataset_dir() function, the
CLIFF_EVAL_DATASET_DIR environment variable is returned as a raw Path(override), so
relative overrides depend on the current working directory and can point to the wrong
dataset. Refactor dataset_dir() to either reject non-absolute overrides with a clear
error message or resolve relative overrides against a known base path (e.g., the repo
root/tests/agents/eval parent), and ensure the final path exists before returning. This
will make load_cases() (lines ~47-48) reliably locate <agent>.jsonl from the intended
dataset directory.
| result.graded_rates = { | ||
| metric: (sum(vals) / len(vals) if vals else 1.0) | ||
| for metric, vals in graded.items() |
There was a problem hiding this comment.
run_enricher_eval treats empty cases as 1.0, so zero-case evals can print PASS via EvalRunResult.passed; should we fail fast or force passed=False when n_cases == 0?
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
backend/cliff/evals/runners.py around lines 56-100, inside run_enricher_eval and the
graded_rates computation (lines ~96-99), stop defaulting metric rates to 1.0 when cases
is empty. Either fail fast by raising an exception when n_cases == 0, or explicitly
force EvalRunResult.passed to return False when n_cases == 0 (and make graded_rates
reflect that, e.g., 0.0). Update the logic so an eval with zero loaded cases cannot
print PASS or satisfy any live-test assertions without actually running metrics.
There was a problem hiding this comment.
Commit af806fc addressed this comment by adding an explicit if not cases guard that raises a ValueError before scoring starts. That prevents zero-case evals from falling through to the default 1.0 graded rates and printing PASS.
…ation Closes the "declared but not enforced" budget gap (ADR-0050 §4) and calibrates gating against a production-representative run. - run_agent_measured returns token usage + wall-clock; the runner enforces the registry BudgetSpec: per-case max_tokens/max_duration_s (hard) + max_usd (best-effort via pricing.py), and per-run max_run_tokens/max_run_usd (the runaway-bill stop). A breach fails the eval. The report shows total tokens + $ estimate. - reference_liveness is now ADVISORY (reported, non-gating): production strips dead links before the user sees them, so a dead-link guess is a model-quality signal, not user harm. Structural fabrication stays the hard gate. Verified against the production model (claude-haiku-4-5): PASS, ~11k tok / ~$0.022, gating metrics clear the floor; reference_liveness tracked advisory. Notable finding the eval surfaced: haiku guesses dead doc URLs more than gpt-4o-mini (advisory ~33-50%) — a real, customer-relevant model quirk. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| def _match(model_id: str | None) -> str | None: | ||
| m = (model_id or "").lower() | ||
| return next((k for k in _PRICE_USD_PER_MTOK if k in m), None) |
There was a problem hiding this comment.
_match() does a first-substring search on model_id, so openai/gpt-5-mini collapses to gpt-5 and gets priced against the wrong tier; could we match the model segment exactly or prefer the longest key?
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
backend/cliff/evals/pricing.py around lines 24-26, the `_match(model_id)` function
currently matches by checking whether each price key is a substring of the full
lowercased model id, which incorrectly collapses `openai/gpt-5-mini` into the `gpt-5`
tier. Refactor `_match` to prefer the most specific/longest key and avoid mid-token
substring matches by matching against model segments (e.g., split on `/` and treat
`-`-separated tokens as boundaries) or otherwise requiring boundary/segment-level
equality. Ensure that `gpt-5-mini` matches the `gpt-5-mini` key only (if present) and
does not fall back to `gpt-5` when the id contains `gpt-5-mini`.
There was a problem hiding this comment.
Commit af806fc addressed this comment by changing _match() to collect all substring candidates and choose the longest key. That prevents shorter tiers like gpt-5 from winning when a more specific key such as gpt-5-mini is present.
- evaluators: dotted-rule jargon regex no longer matches semantic versions like "2.17.1" (CodeRabbit major); drop the redundant http branch in assess_references (Baz). - cases: `expected` is a typed `Expected` model (extra=forbid) so malformed rows fail in load_cases instead of slipping past the evaluators (Baz); `tier` narrowed to the `Tier` literal (CodeRabbit); `CLIFF_EVAL_DATASET_DIR` is resolved to an absolute path so a relative override can't depend on cwd (Baz). - adapter: honor `spec.default_model` when no model_id (CodeRabbit major); validate output is `spec.output_type` (CodeRabbit); narrow `Any` -> typed Finding/BaseModel boundary (CodeRabbit major). - registry: BudgetSpec + AgentEvalSpec are Pydantic models, frozen (CodeRabbit). - runners: 0 cases now raises instead of silently reporting PASS (Baz); pass the typed expected to evaluators via as_dict(). - pricing: longest-key match so e.g. gpt-4o-mini isn't priced as gpt-4o (Baz). - conftest: widen the live-LLM key gate to all build_model providers (OpenRouter/Gemini/Ollama), not just OpenAI/Anthropic (Baz). Declined: extracting a shared run_agent/run_no_tools_agent helper — they return different shapes (raw output object vs post-processed dict) and the shared plumbing is ~2 lines (simplicity rule). Verified: ruff clean, CI harness 9/9, 104 deterministic agent tests, and the live eval vs claude-haiku-4-5 PASS (the regex fix lifted no_jargon_title to 100% by dropping a semantic-version false positive). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review addressed (commit af806fc)All threads from Baz + CodeRabbit, fixed except one principled decline. Fixed:
Declined (with reason): extracting a shared Verified: ruff clean · CI harness 9/9 · 104 deterministic agent tests · live eval vs |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/cliff/evals/evaluators.py (1)
119-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize nullable CVSS range bounds before numeric comparison.
Line 124 can raise at runtime when
expectedcarries explicitnullforcvss_min/cvss_max(currently allowed by the typed case model), because the code comparesNonewithfloat.Suggested fix
if "cvss_min" in expected or "cvss_max" in expected: - lo = expected.get("cvss_min", 0.0) - hi = expected.get("cvss_max", 10.0) + lo_raw = expected.get("cvss_min") + hi_raw = expected.get("cvss_max") + lo = 0.0 if lo_raw is None else float(lo_raw) + hi = 10.0 if hi_raw is None else float(hi_raw) + if lo > hi: + return False, f"invalid cvss range [{lo},{hi}]" if got is None: return False, f"cvss expected in [{lo},{hi}], got null" return (lo <= float(got) <= hi), f"cvss {got} in [{lo},{hi}]"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cliff/evals/evaluators.py` around lines 119 - 124, The CVSS range check in evaluators.py can break when expected["cvss_min"] or expected["cvss_max"] are explicitly null; update the block that handles "cvss_min"/"cvss_max" to normalize nullable bounds before numeric comparison: fetch values from expected with expected.get("cvss_min") and expected.get("cvss_max"), treat None the same as the defaults (0.0 and 10.0) before assigning to lo and hi, and ensure you convert lo and hi to floats only after applying the defaults; keep the existing got None check and the return shape used in this function (the block that currently uses lo = expected.get("cvss_min", 0.0) / hi = expected.get("cvss_max", 10.0) and the final comparison).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/cliff/evals/runners.py`:
- Around line 132-139: The cost check is using the raw argument model_id which
can be None, causing USD caps to be skipped; change calls to estimate_cost_usd
and the subsequent budget comparison to use the resolved/default model (e.g.,
resolved_model = model_id or spec.default_model) instead of model_id when
computing case_cost for functions like estimate_cost_usd(model_id,
run.input_tokens, run.output_tokens) and when checking budget.max_usd and
appending to result.budget_failures (also fix the same pattern around the later
block at the similar case handling near the 163-168 region); ensure you
reference resolved_model everywhere the pricing and cap comparison occur so None
never bypasses enforcement.
---
Outside diff comments:
In `@backend/cliff/evals/evaluators.py`:
- Around line 119-124: The CVSS range check in evaluators.py can break when
expected["cvss_min"] or expected["cvss_max"] are explicitly null; update the
block that handles "cvss_min"/"cvss_max" to normalize nullable bounds before
numeric comparison: fetch values from expected with expected.get("cvss_min") and
expected.get("cvss_max"), treat None the same as the defaults (0.0 and 10.0)
before assigning to lo and hi, and ensure you convert lo and hi to floats only
after applying the defaults; keep the existing got None check and the return
shape used in this function (the block that currently uses lo =
expected.get("cvss_min", 0.0) / hi = expected.get("cvss_max", 10.0) and the
final comparison).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 505dc90d-f340-418b-9b7b-46ec5483d18c
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
backend/cliff/evals/adapter.pybackend/cliff/evals/cases.pybackend/cliff/evals/evaluators.pybackend/cliff/evals/pricing.pybackend/cliff/evals/registry.pybackend/cliff/evals/runners.pybackend/pyproject.tomlbackend/tests/agents/conftest.pybackend/tests/agents/test_evals_harness.py
Correctness: - conftest: the widened live-LLM gate (prev commit) un-skipped live tests for Ollama/OpenRouter/Gemini-only hosts, but eval_model() only maps Anthropic else gpt-4o-mini -> those hosts hard-errored instead of skipping. FIX: gate + model selection now share one policy (cliff.evals.models.eval_runnable / select_eval_model) so they can't diverge; an Ollama-only host with no override skips cleanly; OpenRouter/Gemini auto-select a runnable model. - evaluators: the dotted-rule jargon regex flagged legit titles (domains like www.example.com, frameworks like asp.net.core.mvc). FIX: require a hyphen/ underscore in the run (real scanner rule ids have one) so only rule paths match. Altitude / cleanup: - Move model+env selection out of tests/eval_utils into the public cliff/evals/models.py so the private cliff-os/eval runner shares it instead of re-implementing (kills the cross-repo drift the helper exists to prevent). - registry: drop dead config (live_only, eval_frozen — nothing read them); soften the supported_assertions docstring (the runner doesn't yet validate per-case assertions — cases don't declare them). - adapter: rename the AgentRun measurement struct -> MeasuredRun (it shadowed the domain cliff.models.AgentRun used in ~20 files). - cases: load_cases hints at CLIFF_EVAL_DATASET_DIR when the (wheel-excluded) default sample dir is missing. - pricing: docstring is honest that an unpriced sub-model falls back to its nearest priced sibling (over-estimate; token cap is the hard limit). Verified: ruff clean, CI harness 9/9, 104 deterministic agent tests, live eval vs claude-haiku-4-5 PASS (no_jargon_title 100% with the regex fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
estimate_cost_usd was called with the raw model_id, which can be None when the caller relies on the adapter's default — silently skipping the USD caps. Price against `model_id or spec.default_model` (what the adapter actually runs), and set finding_enricher's default_model to the production default (anthropic/claude-haiku-4-5) so the fallback + pricing are both real. The other open threads were already addressed in prior commits (redundant http branch removed, conftest gate now shares eval_runnable across all providers, empty-cases now raises) and one is a standing decline (run_agent vs run_no_tools_agent — different return shapes, ~2 shared lines). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Latest review pass (commit 5cf5f9a)Fixed (new, CodeRabbit major): USD budget was priced against the raw Already addressed in earlier commits (stale threads):
Standing decline: Verified: ruff clean · CI harness 9/9 · 104 deterministic agent tests · live eval vs haiku PASS. |
| if override := env.get("CLIFF_EVAL_MODEL"): | ||
| return override |
There was a problem hiding this comment.
CLIFF_EVAL_MODEL makes eval_runnable() return true before checking provider env, so live tests can un-skip and then fail in build_model(); should we validate the selected model against available creds 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
backend/cliff/evals/models.py around lines 37-57, the select_eval_model and
eval_runnable logic currently returns CLIFF_EVAL_MODEL unconditionally (lines 45-46), so
eval_runnable becomes true even when no provider credentials are available. Refactor so
that when CLIFF_EVAL_MODEL is set, you validate that the overridden model’s provider
is actually configured in the harvested env (required *_API_KEY and any needed
*_BASE_URL), and only return the override (and thus True for eval_runnable) if the
provider is runnable; otherwise return None/False. If there is an existing
provider/model resolution function used by backend/cliff/agents/runtime/provider.py
(build_model or equivalent), call it (or factor out a shared “resolve model if
configured” helper) so the skip gate matches what tests will execute and never
diverges again.
1 participant
User description
First agent wired under the generic eval system (ADR-0050, cliff-os#7). Rollout order = highest-risk first, so
finding_enricheris the reference implementation that proves the abstraction end-to-end.What's here
A thin layer over
pydantic-evals(already in the tree):cliff/evals/—adapter(drives any workspace agent through one call),registry(AgentEvalSpec: supported assertions + budget + model),cases(typed JSONL loader),evaluators(deterministic — "code first, judge last").reference_verifier.clean_references; a plausible URL that 404s is a graded dead-link metric, not a hard fail (production strips it anyway, and even good models occasionally guess a moved doc URL).test_evals_harness.py— CI, deterministic (FunctionModel/TestModel+ injected http). Proves the scorer is correct without a key, so a green CI means the harness won't silently mis-score.test_evals_finding_enricher.py— live, key-gated (viaconftest). Runs the real enricher overeval/finding_enricher.jsonl: hard gates zero-tolerance, graded metrics on a floor.Test plan / evidence
Notes
## Evaluationsection (the per-agent contract, ADR-0050 Appendix A) goes up as a cliff-os PR for CEO approval.cliff.evals.load_casesresolving atests/path is the documented test/prod-line blur (ADR-0050 Open question feat: Stage 2 — Queue page and Workspace v1 (Phases 4 & 5) #7).🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Deliver an end-to-end agent eval harness that wires
run_agent/run_agent_measuredthroughAgentEvalSpec, budget/pricing guards, custom evaluators, and therun_enricher_evalrunner so thefinding_enricherreference implementation can be exercised deterministically (CI) and with real models (live) under ADR-0050. Coordinate dataset discoverability, live-vs-CI gating, and public sample cases so the same harness drives the open-source deterministic lane, the gated live lane, and the private cliff-os runner while keeping the shared tooling versioned incliff.evals.pyproject.tomlwheel target and editableuv.lockentry) publishes the newcliffpackage so the harness can be consumed from both the repo and downstream eval runner.Modified files (2)
Latest Contributors(1)
cliff.evals.adapter,registry,models,pricing,evaluators, andrunnersinto a single harness that resolves models, budgets, evaluator gates, and scoring loops forfinding_enricherso ADR-0050’s CI/live dual lanes share the same orchestration.Modified files (7)
Latest Contributors(1)
test_evals_harness.py) and live (test_evals_finding_enricher.py) tests so the harness always validates the same flows and datasets the runner uses.Modified files (7)
Latest Contributors(1)