fix(verification): fail loudly on unparsable spec entries and withhold correctness on error - #85
Open
geojaz wants to merge 17 commits into
Open
Conversation
hold was rejected at the schema level, and the only prior implementation sampled after the agent finished. That catches post-run drift but cannot see a safeguard violation the agent commits and then undoes, which is the case that matters: a task whose safeguard forbids dropping a replica count scored full marks on a run that scaled to 2 and back to 4. SafeguardMonitor runs as a daemon thread started before the agent's turn and stopped after it, sampling every hold entry on its own interval and recording the first violation per entry. The violated flag is sticky, so a later passing sample cannot clear it. A check that errors counts as an error rather than a violation, and an entry with zero samples fails rather than silently passing. Sampling cannot see a violation shorter than the poll interval. The interval is tunable via BENCH_HOLD_INTERVAL_SEC and per-entry hold_poll_interval_sec; a watch-based implementation would remove the gap. Signed-off-by: Eric Hole <ehole@onixnet.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: geojaz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @geojaz. 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 Regular contributors should join the org to skip this step. 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
the
needs-ok-to-test
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughHold safeguards now use continuous background sampling during agent execution. The harness converts observations into verification reports, preserves monitor cleanup on failure paths, and marks malformed verification specifications as ChangesHold verification flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant SafeguardMonitor
participant VerificationRunner
participant EvaluationHarness
Agent->>EvaluationHarness: execute task
EvaluationHarness->>SafeguardMonitor: start monitoring
SafeguardMonitor->>VerificationRunner: sample hold entries
VerificationRunner-->>SafeguardMonitor: verification results
Agent-->>EvaluationHarness: task completion or exception
EvaluationHarness->>SafeguardMonitor: stop and collect observations
EvaluationHarness->>EvaluationHarness: build hold reports and status
Suggested reviewers: 🚥 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
size/XXL
janetkuo
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/evalharness/default.py`:
- Around line 590-595: Update _hold_report_entry in
devops_bench/evalharness/default.py (lines 590-595) to add an obs.error_count
branch before the pass branch, returning status "error" when sampling errors
occurred and no violation was observed; preserve status "fail" for observed
violations. Update tests/unit/evalharness/test_verification_wiring.py (lines
363-376) to set error_count=0 for the passing case and add coverage asserting
errored hold observations report status "error".
In `@devops_bench/evalharness/safeguard_monitor.py`:
- Line 61: Validate BENCH_HOLD_INTERVAL_SEC when initializing
HOLD_POLL_INTERVAL_SEC, catching non-numeric and non-finite values and rejecting
values less than or equal to zero. Raise a clear configuration error for invalid
configuration while preserving the existing positive interval behavior.
🪄 Autofix
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 Plus
Run ID: 533515e0-185f-4b81-9ecd-fe8d970dfa62
📒 Files selected for processing (13)
devops_bench/evalharness/default.pydevops_bench/evalharness/safeguard_monitor.pydevops_bench/verification/rollup.pydevops_bench/verification/runner.pydevops_bench/verification/spec.pytests/unit/evalharness/test_default_harness.pytests/unit/evalharness/test_safeguard_monitor.pytests/unit/evalharness/test_verification_wiring.pytests/unit/metrics/test_metrics_verification.pytests/unit/verification/test_combinators.pytests/unit/verification/test_entries.pytests/unit/verification/test_rollup.pytests/unit/verification/test_run_entry.py
mode: hold is no longer safeguard-specific: an objective-role hold entry needs a different driver (a post-run soak) that will live in the same module. Rename the module and its test file, keep the SafeguardMonitor class name (it still accurately describes the safeguard driver), and update the module docstring plus every import/doc reference to describe hold mode generally and name both drivers.
SafeguardMonitor._sample_one inlined the fold of a sample result into a HoldObservation. Pull that into a module-level _fold_sample so the upcoming post-run objective driver can share the exact same fold instead of duplicating it. No behavior change.
An errored sample was counted in error_count but never affected the verdict: a window with 49 errored samples and 1 clean pass scored identically to 50 clean passes. Add HoldObservation.last_sample_status, set on every fold, and a shared hold_verdict() that treats an error recovered within the window as observation noise but a window that ends on an error as never having been actually observed, and scores that as an error rather than a pass. Rewire _hold_report_entry to call hold_verdict instead of its own inline check.
An objective-role hold entry starts false and must become true and stay true. Sampling it live during the agent's turn (SafeguardMonitor) is wrong: the first sample fails before the agent has done anything and latches a permanent violation. run_hold_window samples synchronously on the caller's thread after the agent's turn ends instead, for up to window_sec, bounded by the caller's overall deadline so one entry's soak cannot overrun the shared post-run verification budget. It reuses _fold_sample so both drivers score identically, and it does not stop early on a violation so the report can show whether the entry recovered (the verdict stays fail regardless).
Add hold_window_sec to VerificationEntry and enforce it in _check_role_and_mode: an objective in hold mode now requires hold_window_sec (no default, since a silent default would quietly consume the shared post-run verification budget on every task in a suite), and a safeguard in hold mode must not set it, since its window is always the agent's turn and the field would be silently ignored. Update the pre-existing hold parsing tests that predate hold_window_sec to supply it, and add tests for both new rejection paths.
A safeguard-role hold entry keeps going to the live SafeguardMonitor, started before execute_agent and stopped after, exactly as before, but now only that subset is constructed with it. An objective-role hold entry is excluded from the live monitor (sampling it live would fail on the first sample and latch a permanent violation before the agent has done anything) and is instead soaked synchronously via run_hold_window inside _run_verification, against the same VERIFICATION_TOTAL_BUDGET_SEC deadline every other entry in that pass shares. Both paths still produce a HoldObservation scored through the same _hold_report_entry. This closes the landmine where role: objective, mode: hold validated cleanly but routed to the live monitor and produced near-guaranteed spurious failures.
Cover every branch of hold_verdict (zero samples, all-errored, a window that errors mid-way but recovers and ends clean, a window that ends on an error even after recovering earlier, a violation, a clean pass with absorbed errors), run_hold_window continuing to sample past a violation and stopping at the caller's deadline, and the landmine regression: an objective-role hold entry must be routed to run_hold_window and must never reach the live SafeguardMonitor.
hold_verdict checked last_sample_status == "error" before violated, so a genuine violation followed by a single trailing error sample reported "error" instead of "fail". Downstream scoring treats "error" as nulling a task's correctness entirely, while "fail" scores as a fail, so this let a confirmed violation drop out of scoring instead of failing the task. A violation is a positive observation: losing observability afterward does not un-observe it. Swap the two checks so violated is evaluated first. Add a test covering the previously-masked case (violated and last_sample_status == "error" together must report "fail").
…tion The objective-hold branch guarded a float | None value passed to a float parameter with a bare assert. python -O strips asserts, so the guard would silently disappear under optimization. Replace it with a real conditional raise that survives -O, matching the ValueError style used elsewhere in the module.
hold_verdict() previously reported "error" whenever the window's last sample errored, so a single transient kubectl blip on the final poll of one hold objective was enough to null that entry. Downstream, an "error" objective nulls a task's whole correctness score (not just its own contribution), which made the benchmark oversensitive to isolated flakes and biased toward tasks that happen to hit them. Add HOLD_TRAILING_ERROR_SAMPLES (2) and track a trailing_error_count on HoldObservation, incremented on consecutive errors and reset on any non-error sample. hold_verdict() now only reports the trailing-error case when that count reaches the threshold; a single trailing error is absorbed the same as any other recovered error. A single sample that errors is still caught by the existing all-errored rule, and the violated-before-trailing-error check order is unchanged.
Both SafeguardMonitor._sample_one and run_hold_window hand-rolled the same four-line block for folding an exception raised during sampling into a HoldObservation, duplicating the bookkeeping that lives in _fold_sample for the in-band status == "error" case. A future field addition to HoldObservation would require editing all three sites, and missing one would silently diverge the two drivers. Pull that block into a module-level _fold_error_sample helper next to _fold_sample, and have both drivers call it. _fold_sample's own status == "error" path now delegates to the same helper instead of repeating the bookkeeping a third time. No behavior change.
A non-numeric value used to raise a bare ValueError deep inside module import, and a zero or negative value was accepted silently and made the scheduler spin without sleeping. Add _positive_float_env() to parse and validate the override, raising ConfigError with a message that names the variable and its offending value when it is not a finite number greater than zero. Addresses a CodeRabbit review finding on PR kubernetes-sigs#84.
The comment above HOLD_POLL_INTERVAL_SEC claimed BENCH_VERIFY_TIMEOUT_SEC and BENCH_VERIFY_TOTAL_BUDGET_SEC as env var precedent in scenario.py. Neither exists on this branch: VERIFICATION_TIMEOUT_SEC and VERIFICATION_TOTAL_BUDGET_SEC in scenario.py are plain hardcoded constants, not environment lookups. Correct the comment to reference those constants accurately instead.
A spec that partially failed to parse used to fold the parse-error count into the objective denominator as a fail-closed fraction, producing a normal-looking correctness score computed over a spec nobody had fully seen. rollup() now refuses correctness outright whenever a parse error is present, matching the existing convention of withholding the composite OutcomeScore when VerificationCoverage is declared but correctness never gets emitted. verification_status also flips to a distinct "parse_error" value instead of reading as an ordinary "evaluated" run, and the parse failure now logs at ERROR instead of WARNING. Signed-off-by: Eric Hole <ehole@onixnet.com>
rollup() only forced correctness to None on a whole-spec parse error or an all-errored objective class; a spec where some objectives errored and others evaluated silently scored the remainder as if that were the whole picture, e.g. a run with four of six objectives errored still reported a clean c = 1.000 from the two that happened to evaluate. An errored entry is not an observed pass or fail, so folding the entries that did evaluate into a normal-looking fraction manufactures a score nobody actually earned. Any objective entry ending in status "error" now withholds correctness for the whole entry, mirroring the parse_error_count convention already in this module; the errored count itself was already surfaced via RollupScores.errored and the VerificationCoverage metric. fail-plus-pass objectives are unaffected and still score normally. Signed-off-by: Eric Hole <ehole@onixnet.com>
geojaz
force-pushed
the
ehole/verification-result-integrity
branch
from
August 11, 2026 21:22
b050b65 to
8558075
Compare
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two fixes to keep a broken verification run from producing a confident score.
Spec entries that do not parse now fail loudly. A malformed entry was being skipped, which meant a task could silently verify fewer objectives than it declared and still report a clean result. Skipping an objective is not the same as satisfying it, and the run had no way to tell the difference.
Correctness is withheld when any objective errors. An objective that errored was being folded into the rollup as though it had simply not passed, which let a run with a broken check still land on a definite correctness verdict. An error means the run does not know the answer, and the score should say so rather than guessing.
Both are cases where the failure mode was silence, which is the hardest kind to notice in a benchmark result.
Summary by CodeRabbit
New Features
Bug Fixes
parse_errorstatus.