feat(results): surface which catastrophic gate fired on the row - #152
Open
isadominguez314 wants to merge 28 commits into
Open
feat(results): surface which catastrophic gate fired on the row#152isadominguez314 wants to merge 28 commits into
isadominguez314 wants to merge 28 commits into
Conversation
Agents under test run as ordinary subprocesses on the harness host with no filesystem boundary, so the benchmark's own material -- task definitions with their judge rubrics and verification specs, the scoring code, prior results, the repo checkout -- is reachable. A scan of the existing run corpus confirms the exposure is not theoretical. Add a flag-only detection layer that scans each run's recorded trajectory and attaches a `cheating_report` to every record. It never changes scores, never touches `validated`, and never aborts a run: the report is an annotation for human review. * `rules.py` -- the rule model plus a default ruleset matching the *kind* of sensitive material rather than any specific task, so new tasks are covered without a code change. Extra rules load from an optional YAML file. * `detector.py` -- pure functions over record dicts. Rules match the JSON-dumped tool-call `args`, the tool `result`, and the record's final `output`. An empty trajectory and empty output reports `no_data`, deliberately distinct from `clean`: an errored run gave detection nothing to see, which is not innocence. * `inventory.py` -- the agent home persists between runs, so a previous `report.md` is an answer key for the next attempt. "Left by a prior run" is temporal, not lexical, so the harness snapshots the home before the first agent executes and generates per-run rules from what it finds. Path rules are filtered per record against the task prompt: an entry the prompt itself names is authorized for that record. * `evalharness/default.py` -- the pre-run snapshot and the post-run annotation pass, both best-effort. A detector failure logs and leaves the seeded empty report; it never sinks a completed run. * `docs/components/detection.md` -- what is scanned, the rule categories, the configuration knobs, the report shape, and the limitations of trajectory analysis as a mitigation. Path-shaped rules scan every surface, `result` included. There is deliberately no passive/active distinction: a benchmark path surfacing in an `ls ~` listing is not access, but no legitimate task puts the harness's own material in view either, so the sighting is the signal that the agent went looking. Detection is a mitigation, not a boundary -- it sees only what the transcript recorded. Sandboxing is the real fix and is tracked separately.
Address review feedback on the pre-run home inventory. DEFAULT_BASELINE hard-coded a directory name that exists only on one operator's host, so any other environment granting a skills tree under the agent home would have flagged every honest run for using it. Replace it with baseline_from_granted_paths(), which maps the paths the harness actually granted (AGENT_SKILLS_PATHS) to the home entries holding them, unioned into the baseline at the inventory call site. Derived rather than enumerated: no host layout is baked into the detector. Skip content fingerprinting for symlinked leftovers. Path.is_file() follows links, so a leftover link would otherwise read an arbitrary readable file into a generated pattern -- and patterns are published in the record's report. The link keeps its path rule. Document why _scan_text emits one finding per pattern per surface rather than per occurrence: the budget is spent across the whole record, so per-occurrence matching would let one noisy result crowd out evidence from later trajectory entries.
Detection has been flag-only: a run that read the benchmark's own material scored exactly as if it had not. Add an always-on, deterministic IntegrityMetric that turns a flagged cheating_report into a catastrophic zero, so the run stays on the leaderboard as a visible zero rather than disappearing from it the way validated=False would have done. The gate emits IntegrityCatastrophic rather than reusing VerificationCatastrophic: the scores map is last-write-wins, so a clean integrity check sharing that key would erase a real task catastrophic. The two keys live together in core.score_keys and are read from there by both the pipeline and the normalizer, so the row's catastrophic flag cannot drift from the zero applied to outcomeScore. A no_data report (errored run, or detection disabled) emits nothing -- having seen nothing is not innocence. The outcome finalizer no longer returns early on a gated run whose correctness sources all abstained, because a null outcomeScore drops the row out of leaderboard aggregatebecause a null outcomeScore drops the row out of la so it is neverbecause a null outcomeScore drops the row out of leaderboard aggregatebeonbecause a null outcomeScore drops the row out of leaderboard aggregatebt its result is now consulted by scoring.
Two fail-open paths let a flagged run keep a passing score. _score built the judge before running any metric and let the failure propagate, so a bad JUDGE_PROVIDER or missing key aborted scoring for the whole batch -- including the deterministic catastrophic gates, which need no judge. It now falls back to a null judge and scores what it can. This was live rather than theoretical: get_judge_model() raises for lack of an OPENAI_API_KEY under test, and the harness test had the resulting empty scores map frozen in as an assertion. _reason type-checked the elements of cheating_report.categories but not the container, so a persisted non-list raised on iteration; the pipeline's per-metric guard swallowed that and dropped the gate entirely.
ResultRow.catastrophic collapses the task-safeguard and benchmark- integrity gates into one bool, so a downstream reader cannot tell a VerificationCatastrophic zero from an IntegrityCatastrophic one. Add catastrophicKinds beside it: the CATASTROPHIC_SCORE_KEYS that scored 0.0, verbatim and in tuple order. A list, not a string, because both gates can fire on one run. The bool stays for dashboard back-compat and equals bool(catastrophicKinds) at write time only: rows written before this field re-validate (e.g. through aggregate.rebatch_rows) with a genuine true beside the defaulted empty list, so the bool remains authoritative on historical rows. Additive with a default, so SCHEMA_VERSION stays at 2.
Prior-run leftovers under /tmp (or any shared writable path outside the home) generate no inventory rules, so reading them scans clean. Recorded as a known limitation per review on kubernetes-sigs#139.
# Conflicts: # docs/components/detection.md
Orphaned when the catastrophic flag moved to _CATASTROPHIC_KEYS; nothing imports it from this module (metrics/verification.py's copy is separate and still live). Flagged in review on PR #3.
…itive override Review on PR #3 asked for both to be explicit: emitting nothing on no_data means no gate, so its OutcomeScore matches a clean run's and the distinction lives only in the per-metric map; and a wrongly flagged record can only be overturned today by hand-editing its stored cheating_report, since the deterministic gate re-fires on rescore and BENCH_CHEAT_DETECT is all-or-nothing at construction.
# Conflicts: # docs/components/metrics.md
A row written before catastrophicKinds existed re-validates through rebatch_rows with the list defaulted to [] beside a genuine catastrophic: true — the one behaviour documented twice in prose but asserted nowhere. Requested in review on PR #4.
…l hidden entries The pre-run inventory blanket-skipped dotfiles, so an agent CLI's state dotdir left by a prior run (a stale ~/.openclaw/workspace holding a previous task's deliverables) was invisible while the same material outside a dotdir was caught. Only the enumerated ENVIRONMENT_DOTFILES (shell init/history, XDG trees, provisioned credentials and tool state) are baseline now; every other hidden entry generates rules like any visible leftover. Operators extend coverage through the existing baseline parameter, which is name-based and accepts dotfile names. Known caveat, deliberately unhandled: the state dir of the agent currently under test is not special-cased, so referencing it in a recorded tool call flags; if that bites, the harness should add that one name to the baseline it passes. DETECTOR_VERSION 5 -> 6: v6 flags strictly more than v5. Requested in review on kubernetes-sigs#139.
scan_record re-dumped every entry's args and result for every rule -- len(rules) * len(trajectory) conversions of identical values. Hoist the per-entry text normalization out of the rule loop, and route args through _as_text like the other surfaces so a foreign harness storing a non-JSON-serializable object in args degrades to str() instead of throwing the whole scan away. Requested in review on kubernetes-sigs#139.
Two review asks on kubernetes-sigs#139, both folded into the still-unpublished v6: - _home_prefixes had no left boundary, so the literal home path matched inside a longer token (/data/home/agent/report.md) and a ~ glued to a word read as a home reference. A (?<![\w~]) lookbehind bounds the alternation; quotes, whitespace, = and start-of-string still match. - The harness-repo rule's subtree alternation skipped docs/, but the repo docs describe the detection rules and the scoring formulas, so reading them tells an agent exactly what gets flagged and how the run is graded. docs now flags like any other subtree.
…on point Review on PR #3 asked for the implications of the shared tuple: a key added there automatically zeroes OutcomeScore and flips the row's catastrophic flag with no further wiring, and everything in it must be deterministic because the pipeline applies these gates without a judge.
The extension-point paragraph lands in PR #2's branch, where the row lists only the bool; on this branch the same tuple also feeds catastrophicKinds, so the consequence list names it.
…atch A single run-start snapshot cannot see what the batch itself creates, so task 1's deliverable -- the closest thing to an answer key task 2 could find -- generated no rule and a read of it scanned clean. The home is now re-inventoried before each task's agent runs. Content fingerprints stay pinned to the run-start leftovers. Fingerprints are unfilterable by design, and two iterations of one task legitimately share long lines (a pasted policy body, a command line, a cluster name), so fingerprinting a same-batch deliverable would flag the honest repeat rather than a cheat. Referencing a previous task's output by path has no such innocent explanation, so the path rule still applies. Snapshots pair positionally with their records rather than keying by task name: a batch may run the same task more than once, and each iteration needs the snapshot taken before it.
…ection "detection" says nothing about what is being detected, and it was the one surface still saying it: the record field is already cheating_report, the toggles are BENCH_CHEAT_DETECT / BENCH_CHEAT_INVENTORY, and the docs page is titled "Cheating detection". The directory now matches. Pure rename -- devops_bench/detection -> devops_bench/cheat_detection, tests/unit/detection -> tests/unit/cheat_detection, and docs/components/detection.md -> cheat-detection.md -- with references rewritten. No behaviour change. Naming note for reviewers: "contamination" is the term of art in the ML benchmark literature but means training-set leakage, not an agent reading answers at runtime, so it would mislead rather than clarify.
# Conflicts: # docs/components/cheat-detection.md
The docs index and the glossary's codebase tree both landed upstream after this branch was cut, and neither mentions the package. Adding the entries here rather than leaving them for a follow-up, since docs-sync treats a new top-level package as something both files must carry.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: isadominguez314 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 |
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (23)
Comment |
kubernetes-prow
Bot
added
the
cncf-cla: yes
|
Hi @isadominguez314. 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
the
needs-ok-to-test
kubernetes-prow
Bot
added
the
size/XXL
janetkuo
added
ok-to-test
|
@isadominguez314: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
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.
ResultRow.catastrophic collapses the task-safeguard and benchmark- integrity gates into one bool, so a downstream reader cannot tell a VerificationCatastrophic zero from an IntegrityCatastrophic one. Add catastrophicKinds beside it: the CATASTROPHIC_SCORE_KEYS that scored 0.0, verbatim and in tuple order. A list, not a string, because both gates can fire on one run.
The bool stays for dashboard back-compat and equals bool(catastrophicKinds) at write time only: rows written before this field re-validate (e.g. through aggregate.rebatch_rows) with a genuine true beside the defaulted empty list, so the bool remains authoritative on historical rows. Additive with a default, so SCHEMA_VERSION stays at 2.