Add concrete judge metric families - #34
Conversation
kubernetes-prow
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe metrics package adds checklist, outcome-validity, tool-invocation, grounding, retrieval, chaos, and batch evaluators. It expands public exports, normalizes pipeline contexts, preserves score ordering, isolates metric failures, and adds unit coverage for evaluator behavior and packaged criteria loading. ChangesMetrics evaluation framework
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DetailedResults
participant evaluate_metrics_batch
participant METRICS
participant MetricEvaluator
participant Scores
DetailedResults->>evaluate_metrics_batch: provide result and judge model
evaluate_metrics_batch->>METRICS: select ordered evaluators
METRICS->>MetricEvaluator: evaluate applicable MetricContext
MetricEvaluator->>Scores: return MetricScore entries
evaluate_metrics_batch->>DetailedResults: store scores
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @eugeneng04. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
needs-ok-to-test
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
Adds the five judge metric families that self-register into the METRICS registry: grounding, tool-invocation, checklist, outcome-validity, and chaos metrics. The batch pipeline imports them for their registration side effects and re-exports the checklist helpers, and the package __init__ exposes the full scoring surface. Signed-off-by: Eugene Ng <ngeugene@google.com>
0ce150b to
0435f13
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
devops_bench/verification/spec.py (2)
42-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
json_schema()embeds each member's own$defsinstead of hoisting them to a shared root.Each
m.model_json_schema()call returns a self-contained document with its own local$defs. Placing these raw dicts directly insideanyOf(rather than$ref-pointing into a shared root-level$defs) means any future verifier with a nestedBaseModelfield would produce a$ref: "#/$defs/X"that resolves against the outer combined document root, not its own nested$defs— breaking ref resolution for schema consumers. Pydantic's own multi-model combination helper avoids exactly this by hoisting$defsto the top level and using$refpointers inanyOf.♻️ Proposed fix using `models_json_schema`
+from pydantic.json_schema import models_json_schema + def json_schema() -> dict[str, Any]: members = list(VERIFIERS.values()) - return { - "title": "VerificationSpec", - "anyOf": [m.model_json_schema() for m in members], - "discriminator": {"propertyName": "type"}, - } + _, top_level_schema = models_json_schema( + [(m, "validation") for m in members], title="VerificationSpec" + ) + top_level_schema["anyOf"] = [ + {"$ref": f"`#/`$defs/{m.__name__}"} for m in members + ] + top_level_schema["discriminator"] = {"propertyName": "type"} + return top_level_schemaSince this touches pydantic's schema-combination API, please confirm the exact
models_json_schemasignature/behavior for the pydantic version pinned in this repo before applying.🤖 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 `@devops_bench/verification/spec.py` around lines 42 - 56, Update json_schema() to combine VERIFIERS models using Pydantic’s version-pinned models_json_schema helper rather than embedding each model_json_schema() result directly in anyOf. Confirm the helper’s exact signature and output for the repository’s Pydantic version, then return its combined schema with shared root-level $defs and correctly resolved anyOf references while preserving the VerificationSpec title and type discriminator.
144-190: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUntrusted
type_keyis f-string-interpolated into aPydanticCustomErrormessage template.
_validation_errorbuilds messages likef"unknown verifier type {type_key!r}; ..."and hands the already-interpolated string toPydanticCustomErroras its template. Since that template is documented to be rendered with{curly_brace}placeholders, atypevalue containing literal braces (e.g."{oops}") would leave an unmatched placeholder in the template, risking a rendering error instead of the intendedValidationError. Pydantic's documented pattern is to pass the raw value via thecontextdict instead of pre-formatting it into the template.🛡️ Proposed fix passing values via context
- raise _validation_error( - "verification_spec_unknown_type", - (f"unknown verifier type {type_key!r}; registered: {sorted(VERIFIERS.keys())}"), - input_value=data, - ) from exc + raise _validation_error( + "verification_spec_unknown_type", + "unknown verifier type {type_key!r}; registered: {registered!r}", + input_value=data, + context={"type_key": type_key, "registered": sorted(VERIFIERS.keys())}, + ) from exc(with
_validation_errorextended to accept and forward acontextdict toPydanticCustomError.)Please confirm this rendering behavior against the pydantic-core version pinned in this repo.
🤖 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 `@devops_bench/verification/spec.py` around lines 144 - 190, Update the unknown-type handling in the verifier validation flow and _validation_error so untrusted type_key values are passed to PydanticCustomError through its context rather than interpolated into the message template. Extend _validation_error to accept and forward context, use a static template with a named placeholder for the verifier type, and preserve the resulting ValidationError behavior for values containing literal braces. Confirm the implementation matches the pinned pydantic-core API.tests/unit/verification/test_base.py (1)
22-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest functions are missing return type annotations. Both files' test functions omit
-> None, the shared root cause being no consistent return-type annotation on any test function in this PR's verification test suite.
tests/unit/verification/test_base.py#L22-L66: add-> Nonetotest_default_fields_match_contract,test_compound_result_carries_children_not_raw,test_leaf_result_carries_raw_not_children, andtest_no_details_field.tests/unit/verification/test_verifier_registry.py#L33-L114: add-> Nonetotest_dummy_verifier_parses_without_union_edit,test_unknown_type_lists_registered_keys_in_error,test_already_parsed_unregistered_basemodel_is_rejected, andtest_unregistered_basemodel_as_checks_child_is_rejected.As per path instructions,
tests/**/*.pyshould "Ensure test functions have proper type annotations and clean structure."🤖 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 `@tests/unit/verification/test_base.py` around lines 22 - 66, Add -> None return annotations to test_default_fields_match_contract, test_compound_result_carries_children_not_raw, test_leaf_result_carries_raw_not_children, and test_no_details_field in tests/unit/verification/test_base.py, and to test_dummy_verifier_parses_without_union_edit, test_unknown_type_lists_registered_keys_in_error, test_already_parsed_unregistered_basemodel_is_rejected, and test_unregistered_basemodel_as_checks_child_is_rejected in tests/unit/verification/test_verifier_registry.py; make no other changes.Source: Path instructions
devops_bench/verification/runner.py (1)
152-204: 🩺 Stability & Availability | 🔵 TrivialDesign note: parallel worker threads can outlive the returned result if a leaf ignores its timeout budget.
ex.shutdown(wait=False, cancel_futures=True)only drops queued-but-not-started futures; in-flight workers keep running after_run_parallelreturns. This is fine as documented, contingent on everyBaseVerifier.verifyhonoringtimeout_sec. If a future leaf implementation blocks past its budget (e.g. a raw blocking call with no internal timeout), the non-daemonThreadPoolExecutorthreads can hold the process open past the point this function already reported a timeout. Worth keeping an eye on as new leaf verifiers are added, since this file can't enforce that contract on their behalf.🤖 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 `@devops_bench/verification/runner.py` around lines 152 - 204, Ensure every leaf verifier invoked through _run_parallel and its _run path strictly honors the provided deadline/timeout budget, including blocking operations such as kubectl wait or poll_until. Audit BaseVerifier.verify implementations and add bounded timeouts where needed; preserve the existing parallel result handling while preventing in-flight worker threads from outliving the returned timeout result.tests/unit/verification/test_verifier_registry.py (1)
36-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest cleanup reaches into
Registry._items(private attribute).Popping
VERIFIERS._itemsdirectly couples this test to the registry's internal representation. A publicRegistry.unregister()/context-manager, or a pytest fixture withyield+teardown, would keep the cleanup robust ifRegistry's internals change, and would better satisfy the path instruction to ensure "clean structure" for test fixtures.As per path instructions,
tests/**/*.pyshould have "proper type annotations and clean structure," which suggests this cleanup could use a public API or fixture instead of a private-attribute reach-through.🤖 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 `@tests/unit/verification/test_verifier_registry.py` around lines 36 - 68, Replace the direct VERIFIERS._items mutation in the test cleanup with the registry’s public unregister or scoped-registration mechanism; if unavailable, add a pytest yield fixture that performs supported teardown. Keep the dummy_check registration isolated and ensure cleanup still runs after both verification scenarios without relying on Registry internals.Source: Path instructions
tests/unit/metrics/test_metrics_pipeline.py (1)
33-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest functions lack return-type annotations across all three new test modules. None of the new test functions (nor their
mocker/caplogfixture parameters) carry type hints, the shared root cause for all three files below.
tests/unit/metrics/test_metrics_pipeline.py#L33-L394: annotate eachdef test_*(...)with-> Noneand type themocker/caplogfixture parameters (e.g.mocker: MockerFixture).tests/unit/metrics/test_metrics_chaos.py#L33-L86: annotatetest_chaos_records_geval_and_perf/test_chaos_defaults_fault_and_survives_eval_errorwith-> Noneand typemocker.tests/unit/metrics/test_metrics_grounding.py#L34-L213: annotate alltest_*functions with-> Noneand typemockeron the GEval-mocked tests.As per path instructions,
tests/**/*.pyshould "Ensure test functions have proper type annotations and clean structure."🤖 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 `@tests/unit/metrics/test_metrics_pipeline.py` around lines 33 - 394, Add -> None return annotations to every test_* function in tests/unit/metrics/test_metrics_pipeline.py (lines 33-394), tests/unit/metrics/test_metrics_chaos.py (lines 33-86), and tests/unit/metrics/test_metrics_grounding.py (lines 34-213); annotate each mocker fixture parameter with MockerFixture and each caplog fixture parameter with the appropriate logging capture fixture type, adding required imports. Keep non-fixture parameters and test behavior unchanged.Source: Path instructions
🤖 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 `@devops_bench/metrics/chaos_metrics.py`:
- Around line 26-31: Update the chaos GEval metric definitions diag_metric and
rec_metric to pass GEVAL_PASS_THRESHOLD explicitly to their threshold
configuration, matching sibling metrics and preserving consistent success
evaluation.
In `@devops_bench/metrics/grounding.py`:
- Around line 67-84: Update the retrieval-rate logic around accessed_docs so
each documentation entry matched in the trajectory increments the accessed count
directly, rather than storing doc_name values in a set. Preserve the existing
doc_name and URL matching guards, and calculate the rate from the number of
matched documentation entries.
In `@devops_bench/metrics/outcome_validity.py`:
- Around line 73-99: Apply complete type annotations across the metrics and test
surfaces: annotate the model parameter in build_outcome_validity_metric within
devops_bench/metrics/outcome_validity.py lines 73-99 and the corresponding model
parameter in devops_bench/metrics/tool_invocation.py lines 59-79; annotate all
helpers, fixture parameters, and test functions with return types including None
in tests/unit/metrics/test_metrics_geval.py lines 26-124; and annotate stub
methods, helpers, nested fake-resource methods, and tests in
tests/unit/metrics/test_metrics_skills.py lines 31-150.
In `@devops_bench/verification/spec.py`:
- Around line 122-134: Update the parse_node doctest to use a verifier type
registered in the repository, or explicitly import the registration path before
constructing the pod_healthy example. Keep the example’s discriminator behavior
and bare-list rejection assertions unchanged, and ensure doctest can resolve the
selected type without relying on unavailable registration.
In `@tests/unit/verification/test_package_import.py`:
- Line 27: Update test_import_pulls_no_heavy_sdks with the required return type
hint and a concise docstring describing what the test verifies, while preserving
its existing test behavior.
---
Nitpick comments:
In `@devops_bench/verification/runner.py`:
- Around line 152-204: Ensure every leaf verifier invoked through _run_parallel
and its _run path strictly honors the provided deadline/timeout budget,
including blocking operations such as kubectl wait or poll_until. Audit
BaseVerifier.verify implementations and add bounded timeouts where needed;
preserve the existing parallel result handling while preventing in-flight worker
threads from outliving the returned timeout result.
In `@devops_bench/verification/spec.py`:
- Around line 42-56: Update json_schema() to combine VERIFIERS models using
Pydantic’s version-pinned models_json_schema helper rather than embedding each
model_json_schema() result directly in anyOf. Confirm the helper’s exact
signature and output for the repository’s Pydantic version, then return its
combined schema with shared root-level $defs and correctly resolved anyOf
references while preserving the VerificationSpec title and type discriminator.
- Around line 144-190: Update the unknown-type handling in the verifier
validation flow and _validation_error so untrusted type_key values are passed to
PydanticCustomError through its context rather than interpolated into the
message template. Extend _validation_error to accept and forward context, use a
static template with a named placeholder for the verifier type, and preserve the
resulting ValidationError behavior for values containing literal braces. Confirm
the implementation matches the pinned pydantic-core API.
In `@tests/unit/metrics/test_metrics_pipeline.py`:
- Around line 33-394: Add -> None return annotations to every test_* function in
tests/unit/metrics/test_metrics_pipeline.py (lines 33-394),
tests/unit/metrics/test_metrics_chaos.py (lines 33-86), and
tests/unit/metrics/test_metrics_grounding.py (lines 34-213); annotate each
mocker fixture parameter with MockerFixture and each caplog fixture parameter
with the appropriate logging capture fixture type, adding required imports. Keep
non-fixture parameters and test behavior unchanged.
In `@tests/unit/verification/test_base.py`:
- Around line 22-66: Add -> None return annotations to
test_default_fields_match_contract,
test_compound_result_carries_children_not_raw,
test_leaf_result_carries_raw_not_children, and test_no_details_field in
tests/unit/verification/test_base.py, and to
test_dummy_verifier_parses_without_union_edit,
test_unknown_type_lists_registered_keys_in_error,
test_already_parsed_unregistered_basemodel_is_rejected, and
test_unregistered_basemodel_as_checks_child_is_rejected in
tests/unit/verification/test_verifier_registry.py; make no other changes.
In `@tests/unit/verification/test_verifier_registry.py`:
- Around line 36-68: Replace the direct VERIFIERS._items mutation in the test
cleanup with the registry’s public unregister or scoped-registration mechanism;
if unavailable, add a pytest yield fixture that performs supported teardown.
Keep the dummy_check registration isolated and ensure cleanup still runs after
both verification scenarios without relying on Registry internals.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 601f6829-fcce-4fd8-813d-33110a924ecb
📒 Files selected for processing (21)
devops_bench/metrics/__init__.pydevops_bench/metrics/_skills.pydevops_bench/metrics/chaos_metrics.pydevops_bench/metrics/checklist.pydevops_bench/metrics/geval.pydevops_bench/metrics/grounding.pydevops_bench/metrics/outcome_validity.pydevops_bench/metrics/pipeline.pydevops_bench/metrics/tool_invocation.pydevops_bench/verification/__init__.pydevops_bench/verification/base.pydevops_bench/verification/runner.pydevops_bench/verification/spec.pytests/unit/metrics/test_metrics_chaos.pytests/unit/metrics/test_metrics_geval.pytests/unit/metrics/test_metrics_grounding.pytests/unit/metrics/test_metrics_pipeline.pytests/unit/metrics/test_metrics_skills.pytests/unit/verification/test_base.pytests/unit/verification/test_package_import.pytests/unit/verification/test_verifier_registry.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (6)
devops_bench/verification/spec.py (2)
42-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
json_schema()embeds each member's own$defsinstead of hoisting them to a shared root.Each
m.model_json_schema()call returns a self-contained document with its own local$defs. Placing these raw dicts directly insideanyOf(rather than$ref-pointing into a shared root-level$defs) means any future verifier with a nestedBaseModelfield would produce a$ref: "#/$defs/X"that resolves against the outer combined document root, not its own nested$defs— breaking ref resolution for schema consumers. Pydantic's own multi-model combination helper avoids exactly this by hoisting$defsto the top level and using$refpointers inanyOf.♻️ Proposed fix using `models_json_schema`
+from pydantic.json_schema import models_json_schema + def json_schema() -> dict[str, Any]: members = list(VERIFIERS.values()) - return { - "title": "VerificationSpec", - "anyOf": [m.model_json_schema() for m in members], - "discriminator": {"propertyName": "type"}, - } + _, top_level_schema = models_json_schema( + [(m, "validation") for m in members], title="VerificationSpec" + ) + top_level_schema["anyOf"] = [ + {"$ref": f"`#/`$defs/{m.__name__}"} for m in members + ] + top_level_schema["discriminator"] = {"propertyName": "type"} + return top_level_schemaSince this touches pydantic's schema-combination API, please confirm the exact
models_json_schemasignature/behavior for the pydantic version pinned in this repo before applying.🤖 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 `@devops_bench/verification/spec.py` around lines 42 - 56, Update json_schema() to combine VERIFIERS models using Pydantic’s version-pinned models_json_schema helper rather than embedding each model_json_schema() result directly in anyOf. Confirm the helper’s exact signature and output for the repository’s Pydantic version, then return its combined schema with shared root-level $defs and correctly resolved anyOf references while preserving the VerificationSpec title and type discriminator.
144-190: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUntrusted
type_keyis f-string-interpolated into aPydanticCustomErrormessage template.
_validation_errorbuilds messages likef"unknown verifier type {type_key!r}; ..."and hands the already-interpolated string toPydanticCustomErroras its template. Since that template is documented to be rendered with{curly_brace}placeholders, atypevalue containing literal braces (e.g."{oops}") would leave an unmatched placeholder in the template, risking a rendering error instead of the intendedValidationError. Pydantic's documented pattern is to pass the raw value via thecontextdict instead of pre-formatting it into the template.🛡️ Proposed fix passing values via context
- raise _validation_error( - "verification_spec_unknown_type", - (f"unknown verifier type {type_key!r}; registered: {sorted(VERIFIERS.keys())}"), - input_value=data, - ) from exc + raise _validation_error( + "verification_spec_unknown_type", + "unknown verifier type {type_key!r}; registered: {registered!r}", + input_value=data, + context={"type_key": type_key, "registered": sorted(VERIFIERS.keys())}, + ) from exc(with
_validation_errorextended to accept and forward acontextdict toPydanticCustomError.)Please confirm this rendering behavior against the pydantic-core version pinned in this repo.
🤖 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 `@devops_bench/verification/spec.py` around lines 144 - 190, Update the unknown-type handling in the verifier validation flow and _validation_error so untrusted type_key values are passed to PydanticCustomError through its context rather than interpolated into the message template. Extend _validation_error to accept and forward context, use a static template with a named placeholder for the verifier type, and preserve the resulting ValidationError behavior for values containing literal braces. Confirm the implementation matches the pinned pydantic-core API.tests/unit/verification/test_base.py (1)
22-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest functions are missing return type annotations. Both files' test functions omit
-> None, the shared root cause being no consistent return-type annotation on any test function in this PR's verification test suite.
tests/unit/verification/test_base.py#L22-L66: add-> Nonetotest_default_fields_match_contract,test_compound_result_carries_children_not_raw,test_leaf_result_carries_raw_not_children, andtest_no_details_field.tests/unit/verification/test_verifier_registry.py#L33-L114: add-> Nonetotest_dummy_verifier_parses_without_union_edit,test_unknown_type_lists_registered_keys_in_error,test_already_parsed_unregistered_basemodel_is_rejected, andtest_unregistered_basemodel_as_checks_child_is_rejected.As per path instructions,
tests/**/*.pyshould "Ensure test functions have proper type annotations and clean structure."🤖 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 `@tests/unit/verification/test_base.py` around lines 22 - 66, Add -> None return annotations to test_default_fields_match_contract, test_compound_result_carries_children_not_raw, test_leaf_result_carries_raw_not_children, and test_no_details_field in tests/unit/verification/test_base.py, and to test_dummy_verifier_parses_without_union_edit, test_unknown_type_lists_registered_keys_in_error, test_already_parsed_unregistered_basemodel_is_rejected, and test_unregistered_basemodel_as_checks_child_is_rejected in tests/unit/verification/test_verifier_registry.py; make no other changes.Source: Path instructions
devops_bench/verification/runner.py (1)
152-204: 🩺 Stability & Availability | 🔵 TrivialDesign note: parallel worker threads can outlive the returned result if a leaf ignores its timeout budget.
ex.shutdown(wait=False, cancel_futures=True)only drops queued-but-not-started futures; in-flight workers keep running after_run_parallelreturns. This is fine as documented, contingent on everyBaseVerifier.verifyhonoringtimeout_sec. If a future leaf implementation blocks past its budget (e.g. a raw blocking call with no internal timeout), the non-daemonThreadPoolExecutorthreads can hold the process open past the point this function already reported a timeout. Worth keeping an eye on as new leaf verifiers are added, since this file can't enforce that contract on their behalf.🤖 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 `@devops_bench/verification/runner.py` around lines 152 - 204, Ensure every leaf verifier invoked through _run_parallel and its _run path strictly honors the provided deadline/timeout budget, including blocking operations such as kubectl wait or poll_until. Audit BaseVerifier.verify implementations and add bounded timeouts where needed; preserve the existing parallel result handling while preventing in-flight worker threads from outliving the returned timeout result.tests/unit/verification/test_verifier_registry.py (1)
36-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest cleanup reaches into
Registry._items(private attribute).Popping
VERIFIERS._itemsdirectly couples this test to the registry's internal representation. A publicRegistry.unregister()/context-manager, or a pytest fixture withyield+teardown, would keep the cleanup robust ifRegistry's internals change, and would better satisfy the path instruction to ensure "clean structure" for test fixtures.As per path instructions,
tests/**/*.pyshould have "proper type annotations and clean structure," which suggests this cleanup could use a public API or fixture instead of a private-attribute reach-through.🤖 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 `@tests/unit/verification/test_verifier_registry.py` around lines 36 - 68, Replace the direct VERIFIERS._items mutation in the test cleanup with the registry’s public unregister or scoped-registration mechanism; if unavailable, add a pytest yield fixture that performs supported teardown. Keep the dummy_check registration isolated and ensure cleanup still runs after both verification scenarios without relying on Registry internals.Source: Path instructions
tests/unit/metrics/test_metrics_pipeline.py (1)
33-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest functions lack return-type annotations across all three new test modules. None of the new test functions (nor their
mocker/caplogfixture parameters) carry type hints, the shared root cause for all three files below.
tests/unit/metrics/test_metrics_pipeline.py#L33-L394: annotate eachdef test_*(...)with-> Noneand type themocker/caplogfixture parameters (e.g.mocker: MockerFixture).tests/unit/metrics/test_metrics_chaos.py#L33-L86: annotatetest_chaos_records_geval_and_perf/test_chaos_defaults_fault_and_survives_eval_errorwith-> Noneand typemocker.tests/unit/metrics/test_metrics_grounding.py#L34-L213: annotate alltest_*functions with-> Noneand typemockeron the GEval-mocked tests.As per path instructions,
tests/**/*.pyshould "Ensure test functions have proper type annotations and clean structure."🤖 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 `@tests/unit/metrics/test_metrics_pipeline.py` around lines 33 - 394, Add -> None return annotations to every test_* function in tests/unit/metrics/test_metrics_pipeline.py (lines 33-394), tests/unit/metrics/test_metrics_chaos.py (lines 33-86), and tests/unit/metrics/test_metrics_grounding.py (lines 34-213); annotate each mocker fixture parameter with MockerFixture and each caplog fixture parameter with the appropriate logging capture fixture type, adding required imports. Keep non-fixture parameters and test behavior unchanged.Source: Path instructions
🤖 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 `@devops_bench/metrics/chaos_metrics.py`:
- Around line 26-31: Update the chaos GEval metric definitions diag_metric and
rec_metric to pass GEVAL_PASS_THRESHOLD explicitly to their threshold
configuration, matching sibling metrics and preserving consistent success
evaluation.
In `@devops_bench/metrics/grounding.py`:
- Around line 67-84: Update the retrieval-rate logic around accessed_docs so
each documentation entry matched in the trajectory increments the accessed count
directly, rather than storing doc_name values in a set. Preserve the existing
doc_name and URL matching guards, and calculate the rate from the number of
matched documentation entries.
In `@devops_bench/metrics/outcome_validity.py`:
- Around line 73-99: Apply complete type annotations across the metrics and test
surfaces: annotate the model parameter in build_outcome_validity_metric within
devops_bench/metrics/outcome_validity.py lines 73-99 and the corresponding model
parameter in devops_bench/metrics/tool_invocation.py lines 59-79; annotate all
helpers, fixture parameters, and test functions with return types including None
in tests/unit/metrics/test_metrics_geval.py lines 26-124; and annotate stub
methods, helpers, nested fake-resource methods, and tests in
tests/unit/metrics/test_metrics_skills.py lines 31-150.
In `@devops_bench/verification/spec.py`:
- Around line 122-134: Update the parse_node doctest to use a verifier type
registered in the repository, or explicitly import the registration path before
constructing the pod_healthy example. Keep the example’s discriminator behavior
and bare-list rejection assertions unchanged, and ensure doctest can resolve the
selected type without relying on unavailable registration.
In `@tests/unit/verification/test_package_import.py`:
- Line 27: Update test_import_pulls_no_heavy_sdks with the required return type
hint and a concise docstring describing what the test verifies, while preserving
its existing test behavior.
---
Nitpick comments:
In `@devops_bench/verification/runner.py`:
- Around line 152-204: Ensure every leaf verifier invoked through _run_parallel
and its _run path strictly honors the provided deadline/timeout budget,
including blocking operations such as kubectl wait or poll_until. Audit
BaseVerifier.verify implementations and add bounded timeouts where needed;
preserve the existing parallel result handling while preventing in-flight worker
threads from outliving the returned timeout result.
In `@devops_bench/verification/spec.py`:
- Around line 42-56: Update json_schema() to combine VERIFIERS models using
Pydantic’s version-pinned models_json_schema helper rather than embedding each
model_json_schema() result directly in anyOf. Confirm the helper’s exact
signature and output for the repository’s Pydantic version, then return its
combined schema with shared root-level $defs and correctly resolved anyOf
references while preserving the VerificationSpec title and type discriminator.
- Around line 144-190: Update the unknown-type handling in the verifier
validation flow and _validation_error so untrusted type_key values are passed to
PydanticCustomError through its context rather than interpolated into the
message template. Extend _validation_error to accept and forward context, use a
static template with a named placeholder for the verifier type, and preserve the
resulting ValidationError behavior for values containing literal braces. Confirm
the implementation matches the pinned pydantic-core API.
In `@tests/unit/metrics/test_metrics_pipeline.py`:
- Around line 33-394: Add -> None return annotations to every test_* function in
tests/unit/metrics/test_metrics_pipeline.py (lines 33-394),
tests/unit/metrics/test_metrics_chaos.py (lines 33-86), and
tests/unit/metrics/test_metrics_grounding.py (lines 34-213); annotate each
mocker fixture parameter with MockerFixture and each caplog fixture parameter
with the appropriate logging capture fixture type, adding required imports. Keep
non-fixture parameters and test behavior unchanged.
In `@tests/unit/verification/test_base.py`:
- Around line 22-66: Add -> None return annotations to
test_default_fields_match_contract,
test_compound_result_carries_children_not_raw,
test_leaf_result_carries_raw_not_children, and test_no_details_field in
tests/unit/verification/test_base.py, and to
test_dummy_verifier_parses_without_union_edit,
test_unknown_type_lists_registered_keys_in_error,
test_already_parsed_unregistered_basemodel_is_rejected, and
test_unregistered_basemodel_as_checks_child_is_rejected in
tests/unit/verification/test_verifier_registry.py; make no other changes.
In `@tests/unit/verification/test_verifier_registry.py`:
- Around line 36-68: Replace the direct VERIFIERS._items mutation in the test
cleanup with the registry’s public unregister or scoped-registration mechanism;
if unavailable, add a pytest yield fixture that performs supported teardown.
Keep the dummy_check registration isolated and ensure cleanup still runs after
both verification scenarios without relying on Registry internals.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 601f6829-fcce-4fd8-813d-33110a924ecb
📒 Files selected for processing (21)
devops_bench/metrics/__init__.pydevops_bench/metrics/_skills.pydevops_bench/metrics/chaos_metrics.pydevops_bench/metrics/checklist.pydevops_bench/metrics/geval.pydevops_bench/metrics/grounding.pydevops_bench/metrics/outcome_validity.pydevops_bench/metrics/pipeline.pydevops_bench/metrics/tool_invocation.pydevops_bench/verification/__init__.pydevops_bench/verification/base.pydevops_bench/verification/runner.pydevops_bench/verification/spec.pytests/unit/metrics/test_metrics_chaos.pytests/unit/metrics/test_metrics_geval.pytests/unit/metrics/test_metrics_grounding.pytests/unit/metrics/test_metrics_pipeline.pytests/unit/metrics/test_metrics_skills.pytests/unit/verification/test_base.pytests/unit/verification/test_package_import.pytests/unit/verification/test_verifier_registry.py
🛑 Comments failed to post (5)
devops_bench/metrics/chaos_metrics.py (1)
26-31: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Chaos GEval metrics should use the project threshold.
diag_metricandrec_metricomitthreshold=GEVAL_PASS_THRESHOLD, so they fall back to DeepEval’s default 0.5 while sibling metrics use 0.8. That makes the chaossuccessflags looser and inconsistent with the rest of the metric suite.🤖 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 `@devops_bench/metrics/chaos_metrics.py` around lines 26 - 31, Update the chaos GEval metric definitions diag_metric and rec_metric to pass GEVAL_PASS_THRESHOLD explicitly to their threshold configuration, matching sibling metrics and preserving consistent success evaluation.devops_bench/metrics/grounding.py (1)
67-84: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Retrieval rate undercounts when docs share an empty/missing
doc_name.
accessed_docsis asetkeyed bydoc_name. When two or more documentation entries lack adoc_name(or happen to share one) but are matched via distinct URLs, they all collapse into the same set entry, solen(accessed_docs)undercounts the actually-accessed guide count. E.g. two docs with onlyurlset, both found in the trajectory, yieldlen(accessed_docs) == 1instead of2, silently deflatingDocRetrievalRate.Count matches directly instead of deduplicating by a non-unique name.
🐛 Proposed fix
- accessed_docs = set() + accessed_count = 0 for doc in documentation: - doc_name = doc.get("doc_name") or "" - doc_name_lower = doc_name.lower() + doc_name_lower = (doc.get("doc_name") or "").lower() url_lower = (doc.get("url") or "").lower() - found_in_trajectory = False for step_str in step_strs: # Guard both substrings on truthiness so a missing name/url (now "") # does not spuriously match every step (``"" in s`` is always True). if (doc_name_lower and doc_name_lower in step_str) or ( url_lower and url_lower in step_str ): - found_in_trajectory = True + accessed_count += 1 break - if found_in_trajectory: - accessed_docs.add(doc_name) - return len(accessed_docs) / len(documentation) + return accessed_count / len(documentation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.accessed_count = 0 for doc in documentation: doc_name_lower = (doc.get("doc_name") or "").lower() url_lower = (doc.get("url") or "").lower() for step_str in step_strs: # Guard both substrings on truthiness so a missing name/url (now "") # does not spuriously match every step (``"" in s`` is always True). if (doc_name_lower and doc_name_lower in step_str) or ( url_lower and url_lower in step_str ): accessed_count += 1 break return accessed_count / len(documentation)🤖 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 `@devops_bench/metrics/grounding.py` around lines 67 - 84, Update the retrieval-rate logic around accessed_docs so each documentation entry matched in the trajectory increments the accessed count directly, rather than storing doc_name values in a set. Preserve the existing doc_name and URL matching guards, and calculate the rate from the number of matched documentation entries.devops_bench/metrics/outcome_validity.py (1)
73-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required type annotations across the new metrics surface.
devops_bench/metrics/outcome_validity.py#L73-L99: annotatemodel.devops_bench/metrics/tool_invocation.py#L59-L79: annotatemodel.tests/unit/metrics/test_metrics_geval.py#L26-L124: annotate helpers and every test function, including fixture parameters and-> None.tests/unit/metrics/test_metrics_skills.py#L31-L150: annotate stub methods, helpers, nested fake-resource methods, and tests.As per coding guidelines, “All Python code must include type hints.” As per path instructions, “Ensure test functions have proper type annotations.”
📍 Affects 4 files
devops_bench/metrics/outcome_validity.py#L73-L99(this comment)devops_bench/metrics/tool_invocation.py#L59-L79tests/unit/metrics/test_metrics_geval.py#L26-L124tests/unit/metrics/test_metrics_skills.py#L31-L150🤖 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 `@devops_bench/metrics/outcome_validity.py` around lines 73 - 99, Apply complete type annotations across the metrics and test surfaces: annotate the model parameter in build_outcome_validity_metric within devops_bench/metrics/outcome_validity.py lines 73-99 and the corresponding model parameter in devops_bench/metrics/tool_invocation.py lines 59-79; annotate all helpers, fixture parameters, and test functions with return types including None in tests/unit/metrics/test_metrics_geval.py lines 26-124; and annotate stub methods, helpers, nested fake-resource methods, and tests in tests/unit/metrics/test_metrics_skills.py lines 31-150.Sources: Coding guidelines, Path instructions
devops_bench/verification/spec.py (1)
122-134: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check if doctests are executed in CI/pytest config, and confirm "pod_healthy" registration. rg -n "doctest" pyproject.toml setup.cfg pytest.ini tox.ini 2>/dev/null rg -n 'register\("pod_healthy"\)' -g '*.py'Repository: kubernetes-sigs/devops-bench
Length of output: 166
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- file list ---\n' git ls-files | rg '^(devops_bench/verification/spec\.py|pyproject\.toml|setup\.cfg|pytest\.ini|tox\.ini|\.github/workflows/.*\.ya?ml)$' printf '\n--- spec.py outline ---\n' ast-grep outline devops_bench/verification/spec.py --view expanded || true printf '\n--- search for pod_healthy and registration patterns ---\n' rg -n '"pod_healthy"|pod_healthy|register\(' devops_bench -g '*.py' printf '\n--- test config mentions doctest ---\n' rg -n "doctest|pytest" pyproject.toml setup.cfg pytest.ini tox.ini .github/workflows -g '!**/node_modules/**' || trueRepository: kubernetes-sigs/devops-bench
Length of output: 257
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files | rg '^(devops_bench/verification/spec\.py|pyproject\.toml|setup\.cfg|pytest\.ini|tox\.ini|\.github/workflows/.*\.ya?ml)$'Repository: kubernetes-sigs/devops-bench
Length of output: 215
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- devops_bench/verification/spec.py (around doctest) ---' sed -n '1,220p' devops_bench/verification/spec.py echo echo '--- pyproject.toml ---' sed -n '1,260p' pyproject.toml echo echo '--- search for pod_healthy registration and verifier imports ---' rg -n '"pod_healthy"|pod_healthy|register\(' devops_bench -g '*.py' echo echo '--- search for doctest / pytest config ---' rg -n 'doctest|pytest' pyproject.toml setup.cfg pytest.ini tox.ini .github/workflows -g '!**/node_modules/**' || trueRepository: kubernetes-sigs/devops-bench
Length of output: 11174
Doctest needs a registered verifier. This example uses
pod_healthy, but that type isn’t registered anywhere in this repo, so doctest runs will fail unless the example imports the registration path or switches to a registered verifier.🤖 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 `@devops_bench/verification/spec.py` around lines 122 - 134, Update the parse_node doctest to use a verifier type registered in the repository, or explicitly import the registration path before constructing the pod_healthy example. Keep the example’s discriminator behavior and bare-list rejection assertions unchanged, and ensure doctest can resolve the selected type without relying on unavailable registration.tests/unit/verification/test_package_import.py (1)
27-27: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required type hint and docstring.
As per coding guidelines, Python code must include type hints and public functions must have concise docstrings; the
tests/**/*.pypath instructions also require typed test functions.Suggested fix
-def test_import_pulls_no_heavy_sdks(): +def test_import_pulls_no_heavy_sdks() -> None: + """Verify verification imports without loading provider SDKs."""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def test_import_pulls_no_heavy_sdks() -> None: """Verify verification imports without loading provider SDKs."""🤖 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 `@tests/unit/verification/test_package_import.py` at line 27, Update test_import_pulls_no_heavy_sdks with the required return type hint and a concise docstring describing what the test verifies, while preserving its existing test behavior.Sources: Coding guidelines, Path instructions
- chaos GEval metrics (DiagnosisAccuracy, GracefulRecovery) now pass the shared GEVAL_PASS_THRESHOLD, matching the other judge metrics instead of deepeval's looser default - doc-retrieval rate counts each matched documentation entry directly rather than deduplicating by a possibly-shared or empty doc_name, which undercounted the accessed fraction - annotate the judge model parameter on the outcome-validity and tool-invocation metric builders - add return-type and fixture annotations to the metric unit tests Signed-off-by: Eugene Ng <ngeugene@google.com>
extract_checklist_items used lstrip("- "), a character-class strip that
also removed a leading "--" on flag requirements ("- --dry-run flag" ->
"dry-run flag"). Remove just the single leading "- " bullet via regex and
cover it with a regression test.
Signed-off-by: Eugene Ng <ngeugene@google.com>
kubernetes-prow
Bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eugeneng04, janetkuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
approved
Flip the entries at blob parity with upstream, by upstream PR: - kubernetes-sigs/devops-bench#32 (model clients + CLI agent harnesses): agents/cli/gemini_cli/**, agents/cli/__init__.py - kubernetes-sigs/devops-bench#31 (chaos package): chaos/spec.py, chaos/__init__.py - kubernetes-sigs/devops-bench#29 (verification base + metrics judge): verification/base.py, verification/__init__.py, its base/package tests, metrics/geval.py, metrics/_skills.py, tests/unit/metrics/test_metrics_geval.py - kubernetes-sigs/devops-bench#30 (deployer abstraction + default stack): deployers/base.py, deployers/__init__.py, deployers/noop.py, both engine tests - kubernetes-sigs/devops-bench#34 (concrete judge metric families): metrics/__init__.py Drifted entries stay commented until gke-labs and upstream reconcile.
2 participants
Adds the five judge metric families and wires them into the scoring surface.
Metric families (
devops_bench/metrics/)grounding.py— documentation-grounding scoring and doc-retrieval rate.tool_invocation.py— tool-invocation correctness metric.checklist.py— checklist extraction and scoring.outcome_validity.py— outcome-validity metric.chaos_metrics.py— chaos-scenario scoring.Each family self-registers into the
METRICSregistry on import.pipeline.pyimports them for those registration side effects and re-exports the checklist
helpers, and the package
__init__now exposes the full scoring surface(
evaluate_metrics_batch, the judge accessors, and the per-family builders).Unit tests are co-located under
tests/unit/metrics/: the existingtest_metrics_pipeline.pygains the concrete-family coverage alongside thestub-based dispatch tests, and new tests cover grounding, chaos, and the skill
loaders.
Summary by CodeRabbit
New Features
Tests