From d7357ee17a616ad374e6bb033a4c9adef6e5cc0a Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 07:39:34 -0700 Subject: [PATCH 1/3] feat(review-suite): add connector-outcome curation and promotion tooling ## Summary - Add `curation-record.schema.json` and `promotion-decision.schema.json` under `review-suite/evals/contracts/`, plus `curation.py` and `audit_curation.py` (`just audit-review-curation`) to load and validate them, mirroring the existing corpus/expectation/provenance pattern. - Add the mechanical disclosure guardrail: when a curation record's `provenance.source_class` is `private_authorized`, its public-facing `source_description` must match an allow-listed generic phrase in the new `disclosure-guardrail.json`, and validation fails closed on a path-like token, a bare hostname-shaped token, or a configured deny-listed identifier. The shipped deny-list is empty by design. - Enforce disposition semantics (accepted/rejected/deferred/duplicate/ unresolved validate distinctly), reviewer/private text separation, and promotion-decision rules: unresolved and plain-duplicate records can never be promoted, a global rubric change needs two representative positives plus a negative control, and a repository-instruction change may only target an existing repository-owned instruction file (AGENTS.md or CLAUDE.md) - no new shared path-rule subsystem. - Add synthetic positive, negative, duplicate, unresolved, restricted-data, and promotion-decision fixtures under `review-suite/evals/curation/`, including three fixtures proving the disclosure guardrail fails closed (path-like token, bare hostname, deny-listed identifier), plus 39 new tests in `test_eval_curation.py`. - Document the workflow in `review-suite/evals/curation/README.md` and the root `README.md`. ## Why - #56 requires a conservative, auditable workflow for turning adjudicated connector outcomes into regression cases and, only when evidence warrants it, rubric or repository-instruction updates - proven here as infrastructure with synthetic fixtures only, per its own non-goals. - #56's own disclosure boundary requires the mechanical guardrail to be real, tested, fail-closed code, not a comment or convention, because curation discipline alone has already been shown elsewhere in this repository (`review-suite/evals/baseline/v1/LIMITATIONS.md` items 21-24, 29-31) not to reliably catch every leak on its own. - None of this data resembles or is derived from any real private source; `baseline/v1/` and `v2/` are untouched. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 5 +- README.md | 15 + justfile | 8 + .../contracts/curation-record.schema.json | 160 +++++++ .../evals/contracts/disclosure-guardrail.json | 8 + .../contracts/promotion-decision.schema.json | 121 +++++ review-suite/evals/curation/README.md | 127 +++++ .../invalid/disclosure-bare-hostname.json | 25 + .../disclosure-denylisted-identifier.json | 25 + .../invalid/disclosure-path-token.json | 25 + .../restricted-data-forbidden-field.json | 28 ++ .../promotions/corpus-case-only-example.json | 24 + .../global-rubric-promotion-example.json | 29 ++ .../promotions/no-promotion-example.json | 24 + ...ository-instruction-promotion-example.json | 29 ++ .../records/cache-ttl-false-positive.json | 29 ++ .../curation/records/retry-jitter-defect.json | 29 ++ ...try-jitter-duplicate-with-new-surface.json | 27 ++ .../records/stale-config-schema-defect.json | 29 ++ .../records/unpatched-token-scope-claim.json | 25 + .../webhook-retry-duplicate-claim.json | 26 + review-suite/scripts/evals/audit_curation.py | 92 ++++ review-suite/scripts/evals/curation.py | 450 ++++++++++++++++++ .../scripts/tests/test_eval_curation.py | 345 ++++++++++++++ 24 files changed, 1704 insertions(+), 1 deletion(-) create mode 100644 review-suite/evals/contracts/curation-record.schema.json create mode 100644 review-suite/evals/contracts/disclosure-guardrail.json create mode 100644 review-suite/evals/contracts/promotion-decision.schema.json create mode 100644 review-suite/evals/curation/README.md create mode 100644 review-suite/evals/curation/fixtures/invalid/disclosure-bare-hostname.json create mode 100644 review-suite/evals/curation/fixtures/invalid/disclosure-denylisted-identifier.json create mode 100644 review-suite/evals/curation/fixtures/invalid/disclosure-path-token.json create mode 100644 review-suite/evals/curation/fixtures/invalid/restricted-data-forbidden-field.json create mode 100644 review-suite/evals/curation/promotions/corpus-case-only-example.json create mode 100644 review-suite/evals/curation/promotions/global-rubric-promotion-example.json create mode 100644 review-suite/evals/curation/promotions/no-promotion-example.json create mode 100644 review-suite/evals/curation/promotions/repository-instruction-promotion-example.json create mode 100644 review-suite/evals/curation/records/cache-ttl-false-positive.json create mode 100644 review-suite/evals/curation/records/retry-jitter-defect.json create mode 100644 review-suite/evals/curation/records/retry-jitter-duplicate-with-new-surface.json create mode 100644 review-suite/evals/curation/records/stale-config-schema-defect.json create mode 100644 review-suite/evals/curation/records/unpatched-token-scope-claim.json create mode 100644 review-suite/evals/curation/records/webhook-retry-duplicate-claim.json create mode 100644 review-suite/scripts/evals/audit_curation.py create mode 100644 review-suite/scripts/evals/curation.py create mode 100644 review-suite/scripts/tests/test_eval_curation.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c999f94..4142c78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,12 @@ summary: Chronological history of repository and skill changes. # Changelog -## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, and rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect +## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, and added connector-outcome curation and promotion tooling +- feat(review-suite): add connector-outcome curation and promotion tooling, + including the mechanical disclosure guardrail - docs: fix stale CHANGELOG SHAs left by the main rebase + (`51cc734fc56a97dfa7a754fd046206dd62b375ba`) - docs: backfill the CHANGELOG entry for the review_gate.py canonicalization fix (`e2310bff8cc9c3a38b690a57844436d5357fa471`) - fix: canonicalize review_gate.py through the existing sync-contracts mechanism diff --git a/README.md b/README.md index f60e532..49a6836 100644 --- a/README.md +++ b/README.md @@ -246,6 +246,21 @@ quoting any figure — in particular, **the connector stratum is deferred, not satisfied**, so connector-escape recall has never been measured and no human-review figure may be reported as a connector figure. +### Connector-outcome curation and promotion + +`review-suite/evals/curation/` turns a newly adjudicated connector finding into +a versioned curation record, and turns a group of curation records into a +conservative, evidence-backed promotion decision: a corpus case only, a global +rubric change, a repository-instruction change, or nothing. It is infrastructure +only, proven with synthetic fixtures, and never touches `baseline/v1/` or `v2/`. +See [its README](review-suite/evals/curation/README.md) for the disposition +vocabulary, the mechanical disclosure guardrail that fails closed on a leaked +source identifier, and the promotion rules. + +```bash +just audit-review-curation # curation and promotion-decision integrity, no runtime +``` + ## Prerequisites - Python 3.11+ diff --git a/justfile b/justfile index 0a9239e..06f6d16 100644 --- a/justfile +++ b/justfile @@ -56,6 +56,14 @@ test-review-suite: audit-review-corpus: python3 review-suite/scripts/evals/audit_corpus.py +# Validate connector-outcome curation records and promotion decisions: intake +# schema, disposition vocabulary, duplicate/unresolved handling, provenance and +# retention fields, reviewer/private separation, the mechanical disclosure +# guardrail, and promotion-decision evidence and target rules. Never scrapes +# GitHub, never mutates a review thread, and never launches a model. +audit-review-curation: + python3 review-suite/scripts/evals/audit_curation.py + # Result-blind replay evaluation through an explicit real-runtime executor. # Deliberately excluded from `test`, `lint`, and `check`: this is the only # review-suite command that may spend money. diff --git a/review-suite/evals/contracts/curation-record.schema.json b/review-suite/evals/contracts/curation-record.schema.json new file mode 100644 index 0000000..9b177d3 --- /dev/null +++ b/review-suite/evals/contracts/curation-record.schema.json @@ -0,0 +1,160 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/curation-record.schema.json", + "title": "Connector-outcome curation record", + "description": "One adjudicated connector finding, curated for possible promotion into a result-blind regression case (see `../corpus/corpus.schema.json`) or a rubric/repository-instruction change. A curation record is never itself an executor payload. `public` restates only what a future regression case's reviewer-visible tree may eventually carry; `private` holds grading and administrative evidence exactly as the replay corpus's `private/expectations` and `private/provenance` do, and never enters a reviewer-visible artifact. A connector's claim is untrusted until `disposition` and `public.adjudication_evidence` record how it was actually adjudicated.", + "type": "object", + "additionalProperties": false, + "required": [ + "curation_record_version", + "record_id", + "disposition", + "public", + "private" + ], + "properties": { + "curation_record_version": { "const": "1.0" }, + "record_id": { + "type": "string", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" + }, + "disposition": { + "description": "The final adjudication. Unresolved claims and plain duplicates cannot support a promotion decision; see `promotion-decision.schema.json` and `curation.py` for the cross-record rules JSON Schema cannot express.", + "enum": [ + "accepted_material_defect", + "accepted_acceptance_miss", + "accepted_validation_gap", + "rejected_false_positive", + "rejected_non_causal", + "deferred_hardening", + "duplicate", + "unresolved" + ] + }, + "duplicate_of": { + "description": "Required exactly when `disposition` is `duplicate`; the record_id whose root cause this claim restates.", + "type": "string", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" + }, + "distinct_contribution": { + "description": "Present only alongside `duplicate_of`. Describes the distinct trigger, surface, or negative control this duplicate still contributes, so it can be promoted without double-counting the shared root cause. A duplicate without this field can never appear in a promotion decision's case lists.", + "type": "string", + "minLength": 1 + }, + "public": { + "description": "Everything a future reviewer-visible regression case, or a public tracker artifact, may restate.", + "type": "object", + "additionalProperties": false, + "required": [ + "connector_finding", + "contract_dimension", + "affected_surface", + "adjudication_evidence", + "provenance" + ], + "properties": { + "connector_finding": { + "type": "object", + "additionalProperties": false, + "required": ["claim"], + "properties": { + "claim": { + "description": "The connector's finding, stated as an untrusted claim. It never becomes authoritative merely by appearing here.", + "type": "string", + "minLength": 1 + }, + "connector_identity": { + "type": "string", + "minLength": 1 + } + } + }, + "contract_dimension": { + "description": "The contract/risk dimension this claim concerns (e.g. correctness, solution-simplicity, code-simplicity, disclosure).", + "type": "string", + "minLength": 1 + }, + "affected_surface": { + "type": "string", + "minLength": 1 + }, + "adjudication_evidence": { + "description": "Concise, public evidence for the disposition above. Never the private expected root cause.", + "type": "string", + "minLength": 1 + }, + "reviewer_visible_reproduction": { + "description": "Sanitized reproduction artifacts, safe for a future reviewer-visible packet.", + "type": "array", + "items": { "type": "string", "minLength": 1 } + }, + "provenance": { + "description": "Public-facing provenance only. Retention authority, owner, review date, and fuller source identity are private (see `private.provenance`).", + "type": "object", + "additionalProperties": false, + "required": ["source_class", "source_description"], + "properties": { + "source_class": { + "enum": [ + "public", + "private_authorized", + "repository_history", + "synthetic" + ] + }, + "source_description": { + "description": "When `source_class` is `private_authorized`, this must be an allow-listed generic phrase and must never contain a path-like token, a bare hostname, or a deny-listed identifier. See `disclosure-guardrail.json` and `curation.disclosure_guardrail_errors`, which JSON Schema cannot express.", + "type": "string", + "minLength": 1 + } + } + }, + "corpus_links": { + "description": "Present once this record has actually been promoted into the replay corpus.", + "type": "object", + "additionalProperties": false, + "properties": { + "corpus_version": { "type": "string", "minLength": 1 }, + "evaluation_comparison": { "type": "string", "minLength": 1 } + } + } + } + }, + "private": { + "description": "Never enters a reviewer-visible artifact or an executor payload.", + "type": "object", + "additionalProperties": false, + "required": ["provenance"], + "properties": { + "provenance": { + "type": "object", + "additionalProperties": false, + "required": ["retention_authority", "owner", "review_date"], + "properties": { + "retention_authority": { "type": "string", "minLength": 1 }, + "owner": { "type": "string", "minLength": 1 }, + "review_date": { + "type": "string", + "pattern": "^\\d{4}-\\d{2}-\\d{2}$" + }, + "source_identity": { + "description": "The fuller source identity, kept private. Still subject to the same disclosure discipline as every other repository artifact, but never required to be generic the way `public.provenance.source_description` is.", + "type": "string", + "minLength": 1 + } + } + }, + "expected_root_cause": { + "description": "Required exactly for an accepted disposition; forbidden before adjudication settles it.", + "type": "string", + "minLength": 1 + }, + "accepted_non_finding": { + "description": "Required exactly for a rejected or deferred disposition; forbidden before adjudication settles it.", + "type": "string", + "minLength": 1 + } + } + } + } +} diff --git a/review-suite/evals/contracts/disclosure-guardrail.json b/review-suite/evals/contracts/disclosure-guardrail.json new file mode 100644 index 0000000..c4cd3fd --- /dev/null +++ b/review-suite/evals/contracts/disclosure-guardrail.json @@ -0,0 +1,8 @@ +{ + "guardrail_version": "1.0", + "description": "Configuration for the mechanical disclosure guardrail `curation.disclosure_guardrail_errors` enforces on every curation record whose `public.provenance.source_class` is `private_authorized`. `allowed_source_descriptions` is the exact set of generic phrases a public-facing `source_description` may use. `denylisted_identifiers` is empty by default: this repository's implementing tooling must never learn, guess, or record the identity of any private, owner-authorized source, so no real identifier is shipped here. Add a real known-sensitive identifier (a repository name, internal codename, or hostname) only when the repository owner directly identifies it as one to guard against; this file is intentionally editable without a code change.", + "allowed_source_descriptions": [ + "a private, owner-authorized connector-review source" + ], + "denylisted_identifiers": [] +} diff --git a/review-suite/evals/contracts/promotion-decision.schema.json b/review-suite/evals/contracts/promotion-decision.schema.json new file mode 100644 index 0000000..ebbc5bb --- /dev/null +++ b/review-suite/evals/contracts/promotion-decision.schema.json @@ -0,0 +1,121 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://github.com/shaug/agent-scripts/review-suite/evals/contracts/promotion-decision.schema.json", + "title": "Connector-outcome promotion decision", + "description": "Records the narrowest-owner decision the promotion workflow makes for one or more curation records: keep as a corpus case only, update the global correctness rubric, update a repository-owned instruction file, or promote nothing. `curation.py` enforces every cross-record rule this schema cannot express: which dispositions may support a positive or negative case, representativeness for a rubric change, and that a repository-instruction change only ever targets an existing repository-owned instruction mechanism.", + "type": "object", + "additionalProperties": false, + "required": [ + "promotion_decision_version", + "decision_id", + "decision", + "rationale", + "positive_case_ids", + "negative_control_case_ids", + "evidence" + ], + "properties": { + "promotion_decision_version": { "const": "1.0" }, + "decision_id": { + "type": "string", + "pattern": "^[a-z0-9]+(-[a-z0-9]+)*$" + }, + "decision": { + "enum": [ + "global_rubric_update", + "repository_instruction_update", + "corpus_case_only", + "no_promotion" + ] + }, + "rationale": { "type": "string", "minLength": 1 }, + "positive_case_ids": { + "description": "Curation record ids whose accepted material outcome supports this decision.", + "type": "array", + "items": { "type": "string", "minLength": 1 } + }, + "negative_control_case_ids": { + "description": "Curation record ids whose rejected or deferred tuning outcome supports this decision as a negative control.", + "type": "array", + "items": { "type": "string", "minLength": 1 } + }, + "target": { + "description": "Required exactly when `decision` is `global_rubric_update` or `repository_instruction_update`; forbidden otherwise.", + "type": "object", + "additionalProperties": false, + "required": ["kind", "path", "summary"], + "properties": { + "kind": { "enum": ["global_rubric", "repository_instruction"] }, + "path": { + "description": "Repository-relative path. A `repository_instruction` target must name an existing repository-owned instruction file (AGENTS.md or CLAUDE.md); this ticket ships no new shared path-rule subsystem.", + "type": "string", + "minLength": 1 + }, + "summary": { "type": "string", "minLength": 1 } + } + }, + "evidence": { + "description": "Before/after quality, stability, latency, and available cost evidence from the approved evaluation slice. Required for every decision, including `no_promotion` and `corpus_case_only`, because the measurement is what justifies not changing guidance just as much as it justifies changing it.", + "type": "object", + "additionalProperties": false, + "required": ["before", "after"], + "properties": { + "before": { + "type": "object", + "additionalProperties": false, + "required": [ + "recall", + "false_positive_rate", + "stability", + "latency_seconds", + "cost" + ], + "properties": { + "recall": { + "description": "Left untyped: this repository's schema-subset validator has no numeric type. `curation.py` checks it is actually numeric." + }, + "false_positive_rate": {}, + "stability": {}, + "latency_seconds": {}, + "cost": { + "description": "Either `usd` is reported, or `unavailable` is true with a `reason` - never both, never neither.", + "type": "object", + "additionalProperties": false, + "properties": { + "usd": {}, + "unavailable": { "type": "boolean" }, + "reason": { "type": "string", "minLength": 1 } + } + } + } + }, + "after": { + "type": "object", + "additionalProperties": false, + "required": [ + "recall", + "false_positive_rate", + "stability", + "latency_seconds", + "cost" + ], + "properties": { + "recall": {}, + "false_positive_rate": {}, + "stability": {}, + "latency_seconds": {}, + "cost": { + "type": "object", + "additionalProperties": false, + "properties": { + "usd": {}, + "unavailable": { "type": "boolean" }, + "reason": { "type": "string", "minLength": 1 } + } + } + } + } + } + } + } +} diff --git a/review-suite/evals/curation/README.md b/review-suite/evals/curation/README.md new file mode 100644 index 0000000..0e75275 --- /dev/null +++ b/review-suite/evals/curation/README.md @@ -0,0 +1,127 @@ +# Connector-outcome curation and promotion + +This directory operationalizes future learning from adjudicated connector review +outcomes: it turns a newly adjudicated connector finding into a versioned +curation record, and turns a group of curation records into a conservative, +evidence-backed promotion decision. It does not itself curate any real connector +history. See +[`../contracts/curation-record.schema.json`](../contracts/curation-record.schema.json) +and +[`../contracts/promotion-decision.schema.json`](../contracts/promotion-decision.schema.json) +for the normative schemas, and +[`../../scripts/evals/curation.py`](../../scripts/evals/curation.py) for the +loader and every cross-field rule the schema-subset validator cannot express. + +This is infrastructure, proven with synthetic fixtures. It must never be used to +recreate or re-adjudicate the `baseline/v1/` corpus, and it never touches +`baseline/v1/` or `v2/`, which stay exactly as #58/#59 froze them. + +## Layout + +```text +review-suite/evals/curation/ +├── README.md this file +├── records/.json one curation record per adjudicated claim +├── promotions/.json one promotion decision per record group +└── fixtures/invalid/*.json fixtures that must fail validation, read + directly by tests rather than by the audit + command's normal directory scan +``` + +`just audit-review-curation` loads and validates every record under `records/`, +then every promotion decision under `promotions/` against that loaded set. It +never scrapes GitHub, never mutates a review thread, and never launches a model. + +## Adjudicate first + +A curation record's `disposition` is the actual outcome of adjudication, never a +restatement of the connector's own claim. Eight dispositions validate +distinctly: + +- `accepted_material_defect`, `accepted_acceptance_miss`, + `accepted_validation_gap` - the claim was materially real. Each requires a + private `expected_root_cause`, and only these may support a promotion + decision's `positive_case_ids`. +- `rejected_false_positive`, `rejected_non_causal`, `deferred_hardening` - the + claim was not material, or is deferred tuning evidence. Each requires a + private `accepted_non_finding`, and only these may support + `negative_control_case_ids`. +- `duplicate` - the claim restates another record's root cause. It requires + `duplicate_of`, and can only be promoted if it also declares + `distinct_contribution`: the genuinely distinct trigger, surface, or negative + control that keeps it from double-counting the shared root cause. A plain + duplicate with no distinct contribution can never appear in any promotion + decision. +- `unresolved` - adjudication has not settled. An unresolved claim can never + enter grading expectations or modify active review guidance; it can never + appear in any promotion decision either. + +`record_id` must equal its filename, and `duplicate_of` must reference another +record actually present in the same load. + +## The mechanical disclosure guardrail + +When a record's `public.provenance.source_class` is `private_authorized`, its +`source_description` must be one of the generic phrases in +[`../contracts/disclosure-guardrail.json`](../contracts/disclosure-guardrail.json)'s +`allowed_source_descriptions`, and validation fails closed if it contains a +path-like token (`/`), a bare hostname-shaped token, or any string matching that +file's `denylisted_identifiers`. `denylisted_identifiers` ships empty: this +repository's implementing tooling must never learn, guess, or record the +identity of any private, owner-authorized source, so no real identifier is +shipped here. The repository owner adds a real one directly, without a code +change, if a specific identifier ever needs guarding against. + +`fixtures/invalid/disclosure-path-token.json`, +`fixtures/invalid/disclosure-bare-hostname.json`, and +`fixtures/invalid/disclosure-denylisted-identifier.json` each prove one of these +three failure modes fails closed; see +`review-suite/scripts/tests/test_eval_curation.py`. + +## Reviewer/private separation and restricted data + +A curation record keeps `public` and `private` in one file, because it is +reviewed by a human curator through an ordinary pull request rather than shipped +to an executor - but separation is still enforced: +`curation.reviewer_private_separation_errors` rejects a record whose private +text (retention authority, owner, source identity, expected root cause, accepted +non-finding) appears verbatim in its own public section. + +Private code, secrets, customer data, hidden reasoning, and connector-only +metadata are forbidden everywhere in a curation record. Every object in the +schema sets `additionalProperties: false`, so there is no slot for any of those +categories of data to occupy in the first place; +`fixtures/invalid/restricted-data-forbidden-field.json` proves an attempt to add +one fails closed as an unknown property. + +## Promotion workflow + +1. **Adjudicate first.** An unresolved or plain-duplicate record cannot support + a promotion decision (`curation._promotable_errors`). +2. **Add a regression case.** An accepted material outcome becomes a positive + case; a rejected or deferred tuning outcome becomes a negative control. This + directory never itself creates a `review-suite/evals/corpus/` or `strata/` + case - that population step is separate, later, evidence-backed work. +3. **Measure before changing guidance.** Every promotion decision requires + `evidence.before` and `evidence.after`: recall, false-positive rate, + stability, latency, and either a reported cost or an explicit `unavailable` + reason. This is required even for `no_promotion` and `corpus_case_only`, + because the measurement is what justifies not changing guidance just as much + as it justifies changing it. +4. **Choose the narrowest owner** (`decision`): + - `global_rubric_update` - only for a repository-independent invariant. + Requires at least two positive cases across distinct affected surfaces plus + at least one negative control, and a `target.kind` of `global_rubric`. + - `repository_instruction_update` - for a repository/path-local invariant. + Requires `target.kind` of `repository_instruction`, and `target.path` must + name an existing repository-owned instruction file (`AGENTS.md` or + `CLAUDE.md`) - this workflow ships no new shared path-rule subsystem, with + or without a target file existing yet. + - `corpus_case_only` / `no_promotion` - when no reusable rule is justified. + No `target` is permitted. +5. **Revalidate.** Out of scope for this tooling: a promoted change's actual + preregistered target and non-regression gates are #59/#57's, not this + directory's. + +`records/` and `promotions/` here are worked synthetic examples proving every +rule above is real, tested, and fails closed - not real curated history. diff --git a/review-suite/evals/curation/fixtures/invalid/disclosure-bare-hostname.json b/review-suite/evals/curation/fixtures/invalid/disclosure-bare-hostname.json new file mode 100644 index 0000000..852c635 --- /dev/null +++ b/review-suite/evals/curation/fixtures/invalid/disclosure-bare-hostname.json @@ -0,0 +1,25 @@ +{ + "curation_record_version": "1.0", + "record_id": "disclosure-bare-hostname", + "disposition": "unresolved", + "public": { + "connector_finding": { + "claim": "A connector flagged a possible validation gap.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "synthetic surface for the disclosure guardrail fixture", + "adjudication_evidence": "Fixture proving the disclosure guardrail fails closed on a bare hostname in a public-facing source_description; not a real adjudication.", + "provenance": { + "source_class": "private_authorized", + "source_description": "see connector.internal-tracker.example for details" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/fixtures/invalid/disclosure-denylisted-identifier.json b/review-suite/evals/curation/fixtures/invalid/disclosure-denylisted-identifier.json new file mode 100644 index 0000000..20598d6 --- /dev/null +++ b/review-suite/evals/curation/fixtures/invalid/disclosure-denylisted-identifier.json @@ -0,0 +1,25 @@ +{ + "curation_record_version": "1.0", + "record_id": "disclosure-denylisted-identifier", + "disposition": "unresolved", + "public": { + "connector_finding": { + "claim": "A connector flagged a possible validation gap.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "synthetic surface for the disclosure guardrail fixture", + "adjudication_evidence": "Fixture proving the disclosure guardrail fails closed on a deny-listed identifier in a public-facing source_description. The identifier below (widgetcorp-shadow-connector-history) is entirely fictional, invented only for this test's own explicit deny-list override; it is never present in the shipped `disclosure-guardrail.json` default deny-list, which stays empty because this repository's tooling must never learn or guess a real one.", + "provenance": { + "source_class": "private_authorized", + "source_description": "sourced from widgetcorp-shadow-connector-history" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/fixtures/invalid/disclosure-path-token.json b/review-suite/evals/curation/fixtures/invalid/disclosure-path-token.json new file mode 100644 index 0000000..4630b3c --- /dev/null +++ b/review-suite/evals/curation/fixtures/invalid/disclosure-path-token.json @@ -0,0 +1,25 @@ +{ + "curation_record_version": "1.0", + "record_id": "disclosure-path-token", + "disposition": "unresolved", + "public": { + "connector_finding": { + "claim": "A connector flagged a possible validation gap.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "synthetic surface for the disclosure guardrail fixture", + "adjudication_evidence": "Fixture proving the disclosure guardrail fails closed on a path-like token in a public-facing source_description; not a real adjudication.", + "provenance": { + "source_class": "private_authorized", + "source_description": "details live in connector-reviews/private-corpus, see there" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/fixtures/invalid/restricted-data-forbidden-field.json b/review-suite/evals/curation/fixtures/invalid/restricted-data-forbidden-field.json new file mode 100644 index 0000000..ba22ca2 --- /dev/null +++ b/review-suite/evals/curation/fixtures/invalid/restricted-data-forbidden-field.json @@ -0,0 +1,28 @@ +{ + "curation_record_version": "1.0", + "record_id": "restricted-data-forbidden-field", + "disposition": "unresolved", + "public": { + "connector_finding": { + "claim": "A connector flagged a possible validation gap.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "synthetic surface for the restricted-data fixture", + "adjudication_evidence": "Fixture proving intake validation fails closed on a forbidden restricted-data field; not a real adjudication.", + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + }, + "connector_only_metadata": { + "internal_run_id": "this field is forbidden: connector-only metadata has no slot in the schema" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/promotions/corpus-case-only-example.json b/review-suite/evals/curation/promotions/corpus-case-only-example.json new file mode 100644 index 0000000..17e0e52 --- /dev/null +++ b/review-suite/evals/curation/promotions/corpus-case-only-example.json @@ -0,0 +1,24 @@ +{ + "promotion_decision_version": "1.0", + "decision_id": "corpus-case-only-example", + "decision": "corpus_case_only", + "rationale": "The scheduled-maintenance retry surface shares its root cause with retry-jitter-defect and contributes a genuinely distinct trigger and surface, so it is retained without double-counting - but one additional surface for an already-known rule does not by itself justify a new rubric or repository-instruction change. It becomes a corpus case only. This is a synthetic worked example.", + "positive_case_ids": ["retry-jitter-duplicate-with-new-surface"], + "negative_control_case_ids": [], + "evidence": { + "before": { + "recall": 0.9, + "false_positive_rate": 0.05, + "stability": 0.9, + "latency_seconds": 33.1, + "cost": { "usd": 0.87 } + }, + "after": { + "recall": 0.9, + "false_positive_rate": 0.05, + "stability": 0.9, + "latency_seconds": 33.1, + "cost": { "usd": 0.87 } + } + } +} diff --git a/review-suite/evals/curation/promotions/global-rubric-promotion-example.json b/review-suite/evals/curation/promotions/global-rubric-promotion-example.json new file mode 100644 index 0000000..0e9f263 --- /dev/null +++ b/review-suite/evals/curation/promotions/global-rubric-promotion-example.json @@ -0,0 +1,29 @@ +{ + "promotion_decision_version": "1.0", + "decision_id": "global-rubric-promotion-example", + "decision": "global_rubric_update", + "rationale": "Two accepted material outcomes on distinct surfaces (a config-loader migration miss and a retry-scheduler acceptance miss) share one repository-independent invariant - a candidate's own stated migration/acceptance criterion must actually be enforced, not merely asserted in prose - and a rejected false positive on an unrelated cache TTL gives a negative control that the new rubric wording must not gate on. This is a synthetic worked example proving the promotion schema and its representativeness gate; it does not itself change `review-suite/CONTRACT.md`.", + "positive_case_ids": ["stale-config-schema-defect", "retry-jitter-defect"], + "negative_control_case_ids": ["cache-ttl-false-positive"], + "target": { + "kind": "global_rubric", + "path": "review-suite/CONTRACT.md", + "summary": "Add an explicit correctness invariant: when a candidate's own acceptance criteria or a schema migration state a requirement, verify it is actually enforced by the merged code, not merely asserted in a comment or acceptance-criteria bullet." + }, + "evidence": { + "before": { + "recall": 0.5, + "false_positive_rate": 0.2, + "stability": 0.7, + "latency_seconds": 32.5, + "cost": { "usd": 0.85 } + }, + "after": { + "recall": 0.9, + "false_positive_rate": 0.05, + "stability": 0.9, + "latency_seconds": 33.1, + "cost": { "usd": 0.87 } + } + } +} diff --git a/review-suite/evals/curation/promotions/no-promotion-example.json b/review-suite/evals/curation/promotions/no-promotion-example.json new file mode 100644 index 0000000..26747df --- /dev/null +++ b/review-suite/evals/curation/promotions/no-promotion-example.json @@ -0,0 +1,24 @@ +{ + "promotion_decision_version": "1.0", + "decision_id": "no-promotion-example", + "decision": "no_promotion", + "rationale": "The cache-TTL claim was rejected as a false positive; on its own it is a negative control, not evidence that any guidance is missing or wrong, so nothing is promoted. Measured only to show the workflow's own step 3 (measure before changing guidance) still runs even when the answer is 'no change'. This is a synthetic worked example.", + "positive_case_ids": [], + "negative_control_case_ids": ["cache-ttl-false-positive"], + "evidence": { + "before": { + "recall": 0.8, + "false_positive_rate": 0.1, + "stability": 0.85, + "latency_seconds": 29.4, + "cost": { "unavailable": true, "reason": "measured from a fixture replay, not a paid runtime run" } + }, + "after": { + "recall": 0.8, + "false_positive_rate": 0.1, + "stability": 0.85, + "latency_seconds": 29.4, + "cost": { "unavailable": true, "reason": "measured from a fixture replay, not a paid runtime run" } + } + } +} diff --git a/review-suite/evals/curation/promotions/repository-instruction-promotion-example.json b/review-suite/evals/curation/promotions/repository-instruction-promotion-example.json new file mode 100644 index 0000000..8d5a07a --- /dev/null +++ b/review-suite/evals/curation/promotions/repository-instruction-promotion-example.json @@ -0,0 +1,29 @@ +{ + "promotion_decision_version": "1.0", + "decision_id": "repository-instruction-promotion-example", + "decision": "repository_instruction_update", + "rationale": "The config-loader migration miss is specific to this repository's own schema-migration convention, not a repository-independent invariant, so the narrowest owner is this repository's existing instruction mechanism rather than the global rubric. This is a synthetic worked example proving the promotion schema's existing-mechanism gate; it does not itself edit AGENTS.md.", + "positive_case_ids": ["stale-config-schema-defect"], + "negative_control_case_ids": ["cache-ttl-false-positive"], + "target": { + "kind": "repository_instruction", + "path": "AGENTS.md", + "summary": "Note, beside this repository's existing schema-migration guidance, that a renamed config field must be rejected rather than silently defaulted." + }, + "evidence": { + "before": { + "recall": 0.5, + "false_positive_rate": 0.15, + "stability": 0.75, + "latency_seconds": 30.2, + "cost": { "usd": 0.6 } + }, + "after": { + "recall": 1.0, + "false_positive_rate": 0.1, + "stability": 0.85, + "latency_seconds": 30.8, + "cost": { "usd": 0.62 } + } + } +} diff --git a/review-suite/evals/curation/records/cache-ttl-false-positive.json b/review-suite/evals/curation/records/cache-ttl-false-positive.json new file mode 100644 index 0000000..246f1ea --- /dev/null +++ b/review-suite/evals/curation/records/cache-ttl-false-positive.json @@ -0,0 +1,29 @@ +{ + "curation_record_version": "1.0", + "record_id": "cache-ttl-false-positive", + "disposition": "rejected_false_positive", + "public": { + "connector_finding": { + "claim": "A connector flagged the response cache's TTL constant as too short, claiming it would cause excess upstream load.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "response cache TTL constant", + "adjudication_evidence": "The cited constant is the *client-side* negative-result cache, which the packet's own acceptance criteria intentionally keep short to avoid stale negative results; the connector conflated it with the unrelated, separately-configured upstream cache. No behavior changed and no consequence follows.", + "reviewer_visible_reproduction": [ + "NEGATIVE_CACHE_TTL_SECONDS = 30 is a client-local negative-result cache, not the upstream cache the claim describes" + ], + "provenance": { + "source_class": "private_authorized", + "source_description": "a private, owner-authorized connector-review source" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + }, + "accepted_non_finding": "A short TTL on the client-side negative-result cache is the intended behavior from the packet's own acceptance criteria; the connector's claim conflated it with an unrelated, separately-configured upstream cache and cites no consequence that actually follows." + } +} diff --git a/review-suite/evals/curation/records/retry-jitter-defect.json b/review-suite/evals/curation/records/retry-jitter-defect.json new file mode 100644 index 0000000..374e4ef --- /dev/null +++ b/review-suite/evals/curation/records/retry-jitter-defect.json @@ -0,0 +1,29 @@ +{ + "curation_record_version": "1.0", + "record_id": "retry-jitter-defect", + "disposition": "accepted_acceptance_miss", + "public": { + "connector_finding": { + "claim": "A connector flagged that the webhook retry helper's backoff has no jitter, so a batch of retries can synchronize into a thundering herd.", + "connector_identity": "synthetic-connector-beta" + }, + "contract_dimension": "correctness", + "affected_surface": "webhook retry backoff scheduler", + "adjudication_evidence": "The candidate's own acceptance criteria required exponential backoff \"to reduce retry load\", but the merged implementation used a fixed multiplier with no jitter term, so concurrent retries from one failure window remain synchronized.", + "reviewer_visible_reproduction": [ + "schedule_retry(attempt=3) returns the same delay on every call in the same process" + ], + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + }, + "expected_root_cause": "The retry scheduler's backoff computation omits a jitter term, so retries scheduled from the same failure window remain synchronized rather than spread, missing the candidate's own stated acceptance criterion." + } +} diff --git a/review-suite/evals/curation/records/retry-jitter-duplicate-with-new-surface.json b/review-suite/evals/curation/records/retry-jitter-duplicate-with-new-surface.json new file mode 100644 index 0000000..4aedbea --- /dev/null +++ b/review-suite/evals/curation/records/retry-jitter-duplicate-with-new-surface.json @@ -0,0 +1,27 @@ +{ + "curation_record_version": "1.0", + "record_id": "retry-jitter-duplicate-with-new-surface", + "disposition": "duplicate", + "duplicate_of": "retry-jitter-defect", + "distinct_contribution": "Same missing-jitter root cause, but triggered on the *scheduled maintenance* retry path rather than the webhook retry path retry-jitter-defect covers, giving a second, independent affected surface for the same underlying rule.", + "public": { + "connector_finding": { + "claim": "A connector flagged, on an unrelated candidate, that the scheduled-maintenance retry helper also has no backoff jitter.", + "connector_identity": "synthetic-connector-beta" + }, + "contract_dimension": "correctness", + "affected_surface": "scheduled-maintenance retry scheduler", + "adjudication_evidence": "Same root cause as retry-jitter-defect (missing jitter term in a backoff computation), but on a distinct scheduler surface reached by a distinct trigger (a maintenance window rather than a webhook failure), so it is retained as a second surface rather than folded away as a plain duplicate.", + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/records/stale-config-schema-defect.json b/review-suite/evals/curation/records/stale-config-schema-defect.json new file mode 100644 index 0000000..67d0594 --- /dev/null +++ b/review-suite/evals/curation/records/stale-config-schema-defect.json @@ -0,0 +1,29 @@ +{ + "curation_record_version": "1.0", + "record_id": "stale-config-schema-defect", + "disposition": "accepted_material_defect", + "public": { + "connector_finding": { + "claim": "A connector flagged that the config loader accepts a field renamed in the last schema bump without rejecting the old name.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "config loader schema-version migration", + "adjudication_evidence": "Reproduced by loading a config fixture with the pre-bump field name: the loader silently accepted it and applied a stale default instead of the migrated value. A merge verdict was reached on this candidate before the miss was caught downstream.", + "reviewer_visible_reproduction": [ + "load_config({'old_field_name': 'x'}) returns defaults instead of raising a migration error" + ], + "provenance": { + "source_class": "private_authorized", + "source_description": "a private, owner-authorized connector-review source" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + }, + "expected_root_cause": "The config loader's schema-version migration path does not reject a field name retired by a prior schema bump, so a stale default is silently applied instead of the migrated value." + } +} diff --git a/review-suite/evals/curation/records/unpatched-token-scope-claim.json b/review-suite/evals/curation/records/unpatched-token-scope-claim.json new file mode 100644 index 0000000..b854a58 --- /dev/null +++ b/review-suite/evals/curation/records/unpatched-token-scope-claim.json @@ -0,0 +1,25 @@ +{ + "curation_record_version": "1.0", + "record_id": "unpatched-token-scope-claim", + "disposition": "unresolved", + "public": { + "connector_finding": { + "claim": "A connector flagged that a session-token refresh path may request a broader scope than the original session, without a reproducible trigger.", + "connector_identity": "synthetic-connector-gamma" + }, + "contract_dimension": "correctness", + "affected_surface": "session-token refresh path", + "adjudication_evidence": "Adjudication has not settled: the claim's own reproduction attempt could not confirm the scope actually widens rather than being restated identically, and no independent adjudicator has confirmed or rejected it yet.", + "provenance": { + "source_class": "private_authorized", + "source_description": "a private, owner-authorized connector-review source" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/records/webhook-retry-duplicate-claim.json b/review-suite/evals/curation/records/webhook-retry-duplicate-claim.json new file mode 100644 index 0000000..723526a --- /dev/null +++ b/review-suite/evals/curation/records/webhook-retry-duplicate-claim.json @@ -0,0 +1,26 @@ +{ + "curation_record_version": "1.0", + "record_id": "webhook-retry-duplicate-claim", + "disposition": "duplicate", + "duplicate_of": "retry-jitter-defect", + "public": { + "connector_finding": { + "claim": "A connector flagged, in a second pass over the same candidate, that retries can synchronize because backoff has no randomization.", + "connector_identity": "synthetic-connector-beta" + }, + "contract_dimension": "correctness", + "affected_surface": "webhook retry backoff scheduler", + "adjudication_evidence": "Restates the same root cause already recorded under retry-jitter-defect: the same missing jitter term, on the same scheduler, from the same acceptance criterion. No distinct trigger, surface, or negative control is contributed, so this claim is retained only as a duplicate and must not be separately counted.", + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/scripts/evals/audit_curation.py b/review-suite/scripts/evals/audit_curation.py new file mode 100644 index 0000000..9dff82c --- /dev/null +++ b/review-suite/scripts/evals/audit_curation.py @@ -0,0 +1,92 @@ +#!/usr/bin/env python3 +"""Audit connector-outcome curation records and promotion decisions. + +Checks, in order: + +1. every curation record's schema, disclosure guardrail, reviewer/private + separation, and disposition semantics (`curation.validate_record`); +2. cross-record referential integrity for `duplicate_of` (`curation.load_records`); +3. every promotion decision's schema and cross-record rules + (`curation.validate_promotion_decision`), including which dispositions may + support a positive or negative case, representativeness for a global rubric + change, and that a repository-instruction change only ever targets an + existing repository-owned instruction file. + +Exit status is 0 when every curation record and promotion decision can be +trusted and 1 otherwise. This command never scrapes GitHub, never mutates an +external review thread, and never launches a model. +""" + +from __future__ import annotations + +import argparse +import sys +from pathlib import Path + +if __package__ in (None, ""): + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import curation +else: + from . import curation + + +def audit( + records_root: Path | None = None, promotions_root: Path | None = None +) -> list[str]: + """Return every reason the curation set cannot be trusted, or an empty list.""" + try: + record_set = curation.load_records(records_root) + except curation.CurationError as error: + return [str(error)] + + errors: list[str] = [] + promotions_root = ( + Path(promotions_root) if promotions_root else curation.DEFAULT_PROMOTIONS + ) + if promotions_root.is_dir(): + for path in sorted(promotions_root.glob("*.json")): + try: + document = curation._load_json(path) + except curation.CurationError as error: + errors.append(str(error)) + continue + errors.extend( + f"{path.name}: {error}" + for error in curation.validate_promotion_decision( + document, record_set.records + ) + ) + return errors + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--records", + type=Path, + default=None, + help="curation records directory (default: review-suite/evals/curation/records)", + ) + parser.add_argument( + "--promotions", + type=Path, + default=None, + help=( + "promotion decisions directory " + "(default: review-suite/evals/curation/promotions)" + ), + ) + args = parser.parse_args(argv) + + errors = audit(args.records, args.promotions) + if errors: + for error in errors: + print(error, file=sys.stderr) + print(f"curation audit failed with {len(errors)} error(s)", file=sys.stderr) + return 1 + print("curation audit passed") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/evals/curation.py b/review-suite/scripts/evals/curation.py new file mode 100644 index 0000000..344586b --- /dev/null +++ b/review-suite/scripts/evals/curation.py @@ -0,0 +1,450 @@ +#!/usr/bin/env python3 +"""Connector-outcome curation and promotion-decision contract. + +This module operationalizes future learning from adjudicated connector +findings, kept strictly separate from the result-blind replay corpus that +`corpus.py` owns: + + curation record one adjudicated connector claim, its disposition, its + public and private evidence, and (for a duplicate) why + it is still retained. + promotion decision the narrowest-owner decision a group of curation + records supports: a corpus case only, a global rubric + change, a repository-instruction change, or nothing. + +A curation record is never itself an executor payload and never becomes a +corpus case by this module alone - #56's own non-goals forbid recreating or +re-adjudicating the #58 corpus here. This module only proves the intake and +promotion tooling is real, tested, and fails closed; populating it with real +adjudicated connector history is separate, later work with its own evidence. + +## The disclosure guardrail + +When a curation record's `public.provenance.source_class` is +`private_authorized`, its `source_description` must be one of the generic +phrases in `disclosure-guardrail.json`'s `allowed_source_descriptions`, and +must not contain a path-like token (`/`), a bare hostname, or any string +matching that file's `denylisted_identifiers`. This is a cheap, mechanical +backstop for the disclosure boundary this workflow depends on: curation +discipline alone (choosing a generic description) has already been shown, +elsewhere in this repository, not to reliably catch every leak on its own. +The guardrail is scoped to `private_authorized` records specifically, because +that is the one disclosure risk a generic public description exists to +guard - a `synthetic` or `repository_history` record's description is +already whatever this repository's own history actually is. + +## Reviewer/private separation + +A curation record keeps `public` and `private` in one file rather than three +directories the way a replay-corpus case does, because a curation record is +reviewed by a human curator through an ordinary pull request, not shipped to +an executor. Separation is still enforced, not merely conventional: +`reviewer_private_separation_errors` rejects a record whose private +administrative or grading text (retention authority, owner, source +identity, expected root cause, accepted non-finding) appears verbatim inside +its own public section. +""" + +from __future__ import annotations + +import json +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Any + +from . import protocol + +DEFAULT_RECORDS = protocol.REVIEW_SUITE / "evals" / "curation" / "records" +DEFAULT_PROMOTIONS = protocol.REVIEW_SUITE / "evals" / "curation" / "promotions" +INVALID_FIXTURES = protocol.REVIEW_SUITE / "evals" / "curation" / "fixtures" / "invalid" +GUARDRAIL_CONFIG = protocol.EVAL_CONTRACTS / "disclosure-guardrail.json" + +#: Dispositions an accepted material outcome may declare. These, and only +#: these, may support a promotion decision's `positive_case_ids`. +ACCEPTED_DISPOSITIONS = ( + "accepted_material_defect", + "accepted_acceptance_miss", + "accepted_validation_gap", +) + +#: Dispositions whose rejected/deferred tuning evidence may support a +#: promotion decision's `negative_control_case_ids`. +REJECTED_TUNING_DISPOSITIONS = ( + "rejected_false_positive", + "rejected_non_causal", + "deferred_hardening", +) + +REPOSITORY_INSTRUCTION_BASENAMES = frozenset({"AGENTS.md", "CLAUDE.md"}) + +#: A generic "word.tld"-shaped token. Deliberately permissive: a hostname +#: guardrail exists to fail closed on a plausible leak, not to guess exactly +#: which strings are real hostnames. Requires a two-letter-or-longer final +#: label so short numeric or single-letter abbreviations (e.g. "e.g.", "v1.0") +#: do not trip it. +HOSTNAME_PATTERN = re.compile( + r"[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?\.[a-zA-Z]{2,}" +) + + +class CurationError(ValueError): + """Raised when a curation record or promotion decision cannot be trusted.""" + + +def _load_json(path: Path) -> Any: + if not path.is_file(): + raise CurationError(f"missing {path}") + try: + return json.loads(path.read_text()) + except json.JSONDecodeError as error: + raise CurationError(f"invalid JSON in {path}: {error}") from error + + +def load_guardrail_config(path: Path | None = None) -> dict[str, Any]: + """Load the disclosure guardrail's allow-list and deny-list.""" + return _load_json(Path(path) if path else GUARDRAIL_CONFIG) + + +def disclosure_guardrail_errors( + record: dict[str, Any], *, guardrail: dict[str, Any] | None = None +) -> list[str]: + """Fail closed on a `private_authorized` record's disclosure risk. + + JSON Schema's `enum` could pin the allow-list, but only unconditionally - + this repository's schema-subset validator has no conditional keyword (see + `corpus._provenance_semantics` for the precedent), and the allow-list only + applies when `source_class` is `private_authorized`. Checked here instead. + """ + provenance = record.get("public", {}).get("provenance", {}) + if provenance.get("source_class") != "private_authorized": + return [] + + guardrail = guardrail if guardrail is not None else load_guardrail_config() + description = provenance.get("source_description", "") + errors: list[str] = [] + + allowed = guardrail.get("allowed_source_descriptions", []) + if description not in allowed: + errors.append( + "public.provenance.source_description must be one of the allow-listed " + f"generic phrases for source_class private_authorized: {allowed!r}, got " + f"{description!r}" + ) + if "/" in description: + errors.append( + "public.provenance.source_description contains a path-like token ('/')" + ) + if HOSTNAME_PATTERN.search(description): + errors.append( + "public.provenance.source_description contains a bare hostname-shaped token" + ) + denylist = guardrail.get("denylisted_identifiers", []) + hits = sorted( + {token for token in denylist if token and token.lower() in description.lower()} + ) + if hits: + errors.append( + "public.provenance.source_description matches a deny-listed identifier: " + + ", ".join(hits) + ) + return errors + + +def _private_text_fragments(record: dict[str, Any]) -> list[str]: + private = record.get("private", {}) + provenance = private.get("provenance", {}) + fragments = [ + provenance.get("retention_authority"), + provenance.get("owner"), + provenance.get("source_identity"), + private.get("expected_root_cause"), + private.get("accepted_non_finding"), + ] + return [fragment for fragment in fragments if fragment] + + +def reviewer_private_separation_errors(record: dict[str, Any]) -> list[str]: + """Reject private text that also appears verbatim in the public section.""" + public_text = json.dumps(record.get("public", {})).lower() + errors = [] + for fragment in _private_text_fragments(record): + normalized = fragment.strip().lower() + if normalized and normalized in public_text: + errors.append( + f"private text appears verbatim in the public section: {fragment!r}" + ) + return errors + + +def _disposition_semantics(record: dict[str, Any]) -> list[str]: + """Cross-field disposition rules JSON Schema cannot express. + + Accepted/rejected/deferred, duplicate, and unresolved dispositions must + validate distinctly: an accepted disposition requires the private root + cause a future positive case needs; a rejected or deferred disposition + requires the private accepted non-finding a future negative control needs; + duplicate and unresolved must declare neither, because neither has settled + what a positive or negative case would even record yet. + """ + errors: list[str] = [] + disposition = record.get("disposition") + duplicate_of = record.get("duplicate_of") + distinct_contribution = record.get("distinct_contribution") + private = record.get("private", {}) + + if disposition == "duplicate": + if not duplicate_of: + errors.append("disposition duplicate requires duplicate_of") + elif duplicate_of == record.get("record_id"): + errors.append("duplicate_of cannot reference its own record_id") + else: + if duplicate_of: + errors.append( + f"duplicate_of is only valid when disposition is duplicate, not " + f"{disposition!r}" + ) + if distinct_contribution: + errors.append( + f"distinct_contribution is only valid when disposition is duplicate, " + f"not {disposition!r}" + ) + + if disposition in ACCEPTED_DISPOSITIONS and not private.get("expected_root_cause"): + errors.append( + f"disposition {disposition!r} requires private.expected_root_cause" + ) + if disposition in REJECTED_TUNING_DISPOSITIONS and not private.get( + "accepted_non_finding" + ): + errors.append( + f"disposition {disposition!r} requires private.accepted_non_finding" + ) + if disposition in ("unresolved", "duplicate"): + if private.get("expected_root_cause") or private.get("accepted_non_finding"): + errors.append( + f"disposition {disposition!r} must not declare a private root cause or " + "accepted non-finding before adjudication settles it" + ) + return errors + + +def validate_record(document: dict[str, Any]) -> list[str]: + """Validate one curation record: schema, disclosure, separation, semantics.""" + errors = list(protocol.validate_against("curation-record.schema.json", document)) + if errors: + return errors + errors.extend(disclosure_guardrail_errors(document)) + errors.extend(reviewer_private_separation_errors(document)) + errors.extend(_disposition_semantics(document)) + return errors + + +@dataclass(frozen=True) +class RecordSet: + root: Path + records: dict[str, dict[str, Any]] + + +def load_records(root: Path | None = None) -> RecordSet: + """Load and validate every curation record in a directory, failing closed. + + Cross-record referential integrity (a `duplicate_of` must actually exist) + can only be checked once every record is loaded, so it happens here rather + than in `validate_record`. + """ + root = Path(root) if root else DEFAULT_RECORDS + if not root.is_dir(): + raise CurationError(f"missing curation records directory {root}") + + records: dict[str, dict[str, Any]] = {} + errors: list[str] = [] + for path in sorted(root.glob("*.json")): + document = _load_json(path) + record_id = document.get("record_id") + if record_id != path.stem: + errors.append( + f"{path.name}: record_id {record_id!r} does not match filename" + ) + record_errors = validate_record(document) + if record_errors: + errors.append(f"{path.name}: " + "; ".join(record_errors)) + continue + if record_id in records: + errors.append(f"duplicate record_id {record_id!r}") + continue + records[record_id] = document + + if errors: + raise CurationError("; ".join(errors)) + + for record_id, document in records.items(): + duplicate_of = document.get("duplicate_of") + if duplicate_of and duplicate_of not in records: + errors.append(f"{record_id}: duplicate_of {duplicate_of!r} does not exist") + if errors: + raise CurationError("; ".join(errors)) + + return RecordSet(root=root, records=records) + + +def _promotable_errors( + record_id: str, record: dict[str, Any], *, as_positive: bool +) -> list[str]: + """Whether one record may support a positive or negative promotion slot. + + Unresolved claims can never enter grading expectations or modify active + review guidance. A duplicate without a recorded distinct contribution can + never be promoted either - promoting it would double-count the root cause + it shares with the record it duplicates. + """ + disposition = record.get("disposition") + if disposition == "unresolved": + return [ + f"{record_id}: unresolved claims cannot enter grading expectations or " + "modify active review guidance" + ] + if disposition == "duplicate": + if not record.get("distinct_contribution"): + return [ + f"{record_id}: a duplicate without a distinct trigger, surface, or " + "negative control cannot be promoted without double-counting its root " + "cause" + ] + return [] + if as_positive and disposition not in ACCEPTED_DISPOSITIONS: + return [ + f"{record_id}: disposition {disposition!r} is not an accepted material " + "outcome and cannot support a positive regression case" + ] + if not as_positive and disposition not in REJECTED_TUNING_DISPOSITIONS: + return [ + f"{record_id}: disposition {disposition!r} is not rejected/deferred " + "tuning evidence and cannot support a negative control" + ] + return [] + + +def _metric_semantics(metric: dict[str, Any], label: str) -> list[str]: + errors = [] + for field in ("recall", "false_positive_rate", "stability", "latency_seconds"): + value = metric.get(field) + if isinstance(value, bool) or not isinstance(value, (int, float)): + errors.append(f"evidence.{label}.{field} must be a number") + + cost = metric.get("cost", {}) + has_usd = "usd" in cost + unavailable = cost.get("unavailable") + if has_usd and unavailable: + errors.append(f"evidence.{label}.cost cannot be both reported and unavailable") + elif not has_usd and not unavailable: + errors.append( + f"evidence.{label}.cost must report usd or an explicit unavailable reason" + ) + elif unavailable and not cost.get("reason"): + errors.append(f"evidence.{label}.cost.unavailable requires a reason") + elif has_usd and ( + isinstance(cost.get("usd"), bool) + or not isinstance(cost.get("usd"), (int, float)) + ): + errors.append(f"evidence.{label}.cost.usd must be a number") + return errors + + +def _evidence_semantics(evidence: dict[str, Any]) -> list[str]: + errors = [] + for label in ("before", "after"): + metric = evidence.get(label) + if not isinstance(metric, dict): + errors.append(f"evidence.{label} is required") + continue + errors.extend(_metric_semantics(metric, label)) + return errors + + +def validate_promotion_decision( + document: dict[str, Any], records: dict[str, dict[str, Any]] +) -> list[str]: + """Validate one promotion decision against the loaded curation records.""" + errors = list(protocol.validate_against("promotion-decision.schema.json", document)) + if errors: + return errors + + decision = document["decision"] + target = document.get("target") + positive_ids = document.get("positive_case_ids", []) + negative_ids = document.get("negative_control_case_ids", []) + + for record_id in positive_ids: + if record_id not in records: + errors.append(f"positive_case_ids: unknown record {record_id!r}") + continue + errors.extend( + _promotable_errors(record_id, records[record_id], as_positive=True) + ) + for record_id in negative_ids: + if record_id not in records: + errors.append(f"negative_control_case_ids: unknown record {record_id!r}") + continue + errors.extend( + _promotable_errors(record_id, records[record_id], as_positive=False) + ) + overlap = sorted(set(positive_ids) & set(negative_ids)) + if overlap: + errors.append( + "record(s) listed as both positive and negative: " + ", ".join(overlap) + ) + + requires_target = decision in ( + "global_rubric_update", + "repository_instruction_update", + ) + if requires_target and not target: + errors.append(f"decision {decision!r} requires a target") + if not requires_target and target: + errors.append(f"decision {decision!r} must not declare a target") + + if target: + if decision == "global_rubric_update" and target["kind"] != "global_rubric": + errors.append("global_rubric_update requires target.kind global_rubric") + if ( + decision == "repository_instruction_update" + and target["kind"] != "repository_instruction" + ): + errors.append( + "repository_instruction_update requires target.kind " + "repository_instruction" + ) + + if decision == "global_rubric_update": + valid_positive = [ + record_id for record_id in positive_ids if record_id in records + ] + surfaces = { + records[record_id]["public"]["affected_surface"] + for record_id in valid_positive + } + if len(valid_positive) < 2 or len(surfaces) < 2: + errors.append( + "global_rubric_update requires at least two representative positive " + "cases across distinct affected surfaces, supported by more than one " + "positive plus relevant negative controls" + ) + if not negative_ids: + errors.append("global_rubric_update requires at least one negative control") + + if decision == "repository_instruction_update" and target: + path = target["path"] + if Path(path).name not in REPOSITORY_INSTRUCTION_BASENAMES: + errors.append( + "repository_instruction_update must target an existing " + "repository-owned instruction mechanism (AGENTS.md or CLAUDE.md), not " + f"a new one: {path!r}" + ) + elif not (protocol.REPOSITORY_ROOT / path).is_file(): + errors.append( + f"repository_instruction_update target does not exist: {path!r}" + ) + + errors.extend(_evidence_semantics(document.get("evidence", {}))) + return errors diff --git a/review-suite/scripts/tests/test_eval_curation.py b/review-suite/scripts/tests/test_eval_curation.py new file mode 100644 index 0000000..9676238 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_curation.py @@ -0,0 +1,345 @@ +"""Curation-record and promotion-decision contract tests for #56. + +Covers the intake schema, disposition vocabulary, duplicate/unresolved +handling, provenance/retention fields, reviewer/private separation, the +mechanical disclosure guardrail, and promotion-decision evidence and target +rules - all against synthetic fixtures only. None of this data resembles or +is derived from any real private source. +""" + +from __future__ import annotations + +import copy +import json +import subprocess +import sys +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import audit_curation, curation # noqa: E402 + +INVALID_DIR = curation.INVALID_FIXTURES +AUDIT_SCRIPT = SCRIPTS_DIR / "evals" / "audit_curation.py" + + +def _load(path: Path) -> dict: + return json.loads(path.read_text()) + + +class RecordSetTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.record_set = curation.load_records() + + def test_the_shipped_records_load_and_cover_every_disposition_family(self): + dispositions = { + record["disposition"] for record in self.record_set.records.values() + } + self.assertIn("accepted_material_defect", dispositions) + self.assertIn("accepted_acceptance_miss", dispositions) + self.assertIn("rejected_false_positive", dispositions) + self.assertIn("duplicate", dispositions) + self.assertIn("unresolved", dispositions) + + def test_a_duplicate_declares_the_record_it_restates(self): + duplicate = self.record_set.records["webhook-retry-duplicate-claim"] + self.assertEqual("retry-jitter-defect", duplicate["duplicate_of"]) + self.assertNotIn("distinct_contribution", duplicate) + + def test_a_distinct_duplicate_declares_its_contribution(self): + duplicate = self.record_set.records["retry-jitter-duplicate-with-new-surface"] + self.assertEqual("retry-jitter-defect", duplicate["duplicate_of"]) + self.assertTrue(duplicate["distinct_contribution"]) + + +class RecordSchemaAndSemanticsTests(unittest.TestCase): + def setUp(self): + self.record = _load( + curation.DEFAULT_RECORDS / "stale-config-schema-defect.json" + ) + + def test_a_valid_record_has_no_errors(self): + self.assertEqual([], curation.validate_record(self.record)) + + def test_an_unknown_disposition_fails_schema(self): + self.record["disposition"] = "probably_fine" + errors = curation.validate_record(self.record) + self.assertTrue(any("disposition" in error for error in errors)) + + def test_duplicate_without_duplicate_of_fails(self): + self.record["disposition"] = "duplicate" + del self.record["private"]["expected_root_cause"] + errors = curation.validate_record(self.record) + self.assertTrue(any("duplicate_of" in error for error in errors)) + + def test_duplicate_of_on_a_non_duplicate_disposition_fails(self): + self.record["duplicate_of"] = "some-other-record" + errors = curation.validate_record(self.record) + self.assertTrue( + any("only valid when disposition is duplicate" in error for error in errors) + ) + + def test_accepted_disposition_without_expected_root_cause_fails(self): + del self.record["private"]["expected_root_cause"] + errors = curation.validate_record(self.record) + self.assertTrue(any("expected_root_cause" in error for error in errors)) + + def test_rejected_disposition_without_accepted_non_finding_fails(self): + self.record["disposition"] = "rejected_false_positive" + errors = curation.validate_record(self.record) + self.assertTrue(any("accepted_non_finding" in error for error in errors)) + + def test_unresolved_disposition_with_a_private_root_cause_fails(self): + self.record["disposition"] = "unresolved" + errors = curation.validate_record(self.record) + self.assertTrue( + any("must not declare a private root cause" in error for error in errors) + ) + + def test_private_text_echoed_into_the_public_section_fails(self): + leak = self.record["private"]["expected_root_cause"] + self.record["public"]["adjudication_evidence"] = leak + errors = curation.validate_record(self.record) + self.assertTrue( + any("appears verbatim in the public section" in error for error in errors) + ) + + +class DisclosureGuardrailTests(unittest.TestCase): + """The one piece of code #56 exists to guarantee is real and fails closed.""" + + def test_the_allow_listed_phrase_passes(self): + record = _load(curation.DEFAULT_RECORDS / "stale-config-schema-defect.json") + self.assertEqual([], curation.disclosure_guardrail_errors(record)) + + def test_non_private_authorized_records_are_not_checked(self): + record = _load(curation.DEFAULT_RECORDS / "retry-jitter-defect.json") + record["public"]["provenance"]["source_description"] = ( + "path/like/but/irrelevant" + ) + self.assertEqual([], curation.disclosure_guardrail_errors(record)) + + def test_a_disallowed_generic_phrase_fails_even_without_a_leak_shape(self): + record = _load(curation.DEFAULT_RECORDS / "stale-config-schema-defect.json") + record["public"]["provenance"]["source_description"] = ( + "our usual connector feed" + ) + errors = curation.disclosure_guardrail_errors(record) + self.assertTrue(any("allow-listed" in error for error in errors)) + + def test_path_like_token_fails_closed(self): + record = _load(INVALID_DIR / "disclosure-path-token.json") + errors = curation.disclosure_guardrail_errors(record) + self.assertTrue(any("path-like token" in error for error in errors)) + + def test_bare_hostname_fails_closed(self): + record = _load(INVALID_DIR / "disclosure-bare-hostname.json") + errors = curation.disclosure_guardrail_errors(record) + self.assertTrue(any("hostname" in error for error in errors)) + + def test_denylisted_identifier_fails_closed(self): + record = _load(INVALID_DIR / "disclosure-denylisted-identifier.json") + # The shipped default deny-list is empty by design; this test supplies + # its own synthetic, clearly-fictional entry to prove the mechanism + # without the shipped config ever naming anything real. + guardrail = curation.load_guardrail_config() + guardrail = { + **guardrail, + "denylisted_identifiers": ["widgetcorp-shadow-connector-history"], + } + errors = curation.disclosure_guardrail_errors(record, guardrail=guardrail) + self.assertTrue(any("deny-listed identifier" in error for error in errors)) + + def test_the_shipped_denylist_is_empty_by_default(self): + guardrail = curation.load_guardrail_config() + self.assertEqual([], guardrail["denylisted_identifiers"]) + + def test_record_load_rejects_every_disclosure_fixture(self): + for name in ( + "disclosure-path-token", + "disclosure-bare-hostname", + "disclosure-denylisted-identifier", + ): + with self.subTest(fixture=name): + record = _load(INVALID_DIR / f"{name}.json") + self.assertTrue(curation.validate_record(record)) + + +class RestrictedDataTests(unittest.TestCase): + def test_a_forbidden_field_fails_closed_as_an_unknown_property(self): + record = _load(INVALID_DIR / "restricted-data-forbidden-field.json") + errors = curation.validate_record(record) + self.assertTrue(any("unknown property" in error for error in errors)) + + +class PromotionDecisionTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.records = curation.load_records().records + + def _promotion(self, name: str) -> dict: + return _load(curation.DEFAULT_PROMOTIONS / f"{name}.json") + + def test_every_shipped_promotion_decision_validates_cleanly(self): + for path in sorted(curation.DEFAULT_PROMOTIONS.glob("*.json")): + with self.subTest(promotion=path.name): + document = _load(path) + self.assertEqual( + [], curation.validate_promotion_decision(document, self.records) + ) + + def test_an_unresolved_record_cannot_be_promoted(self): + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["unpatched-token-scope-claim"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("unresolved claims cannot enter" in error for error in errors) + ) + + def test_a_plain_duplicate_cannot_be_promoted(self): + document = self._promotion("no-promotion-example") + document["negative_control_case_ids"] = ["webhook-retry-duplicate-claim"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("double-counting its root cause" in error for error in errors) + ) + + def test_a_distinct_duplicate_may_be_promoted(self): + document = self._promotion("corpus-case-only-example") + self.assertEqual( + [], curation.validate_promotion_decision(document, self.records) + ) + + def test_a_rejected_record_cannot_support_a_positive_case(self): + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["cache-ttl-false-positive"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any( + "cannot support a positive regression case" in error for error in errors + ) + ) + + def test_an_accepted_record_cannot_support_a_negative_control(self): + document = self._promotion("no-promotion-example") + document["negative_control_case_ids"] = ["stale-config-schema-defect"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("cannot support a negative control" in error for error in errors) + ) + + def test_global_rubric_update_requires_two_distinct_surfaces(self): + document = self._promotion("global-rubric-promotion-example") + document["positive_case_ids"] = ["stale-config-schema-defect"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("representative positive cases" in error for error in errors) + ) + + def test_global_rubric_update_requires_a_negative_control(self): + document = self._promotion("global-rubric-promotion-example") + document["negative_control_case_ids"] = [] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("negative control" in error for error in errors)) + + def test_repository_instruction_update_rejects_a_new_subsystem_path(self): + document = self._promotion("repository-instruction-promotion-example") + document["target"]["path"] = "review-suite/evals/curation/PATH-RULES.json" + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("not a new one" in error for error in errors)) + + def test_repository_instruction_update_rejects_a_nonexistent_instruction_file(self): + document = self._promotion("repository-instruction-promotion-example") + document["target"]["path"] = "skills/nonexistent-skill/AGENTS.md" + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("does not exist" in error for error in errors)) + + def test_no_promotion_must_not_declare_a_target(self): + document = self._promotion("no-promotion-example") + document["target"] = { + "kind": "global_rubric", + "path": "review-suite/CONTRACT.md", + "summary": "should not be permitted", + } + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("must not declare a target" in error for error in errors)) + + def test_a_record_cannot_be_both_positive_and_negative(self): + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["cache-ttl-false-positive"] + document["negative_control_case_ids"] = ["cache-ttl-false-positive"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("both positive and negative" in error for error in errors)) + + def test_missing_cost_evidence_fails_closed(self): + document = self._promotion("no-promotion-example") + del document["evidence"]["before"]["cost"]["reason"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("cost.unavailable requires a reason" in error for error in errors) + ) + + def test_reported_and_unavailable_cost_together_fails_closed(self): + document = self._promotion("global-rubric-promotion-example") + document["evidence"]["before"]["cost"]["unavailable"] = True + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("both reported and unavailable" in error for error in errors) + ) + + def test_a_non_numeric_recall_fails_closed(self): + document = self._promotion("global-rubric-promotion-example") + document["evidence"]["before"]["recall"] = "high" + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue(any("recall must be a number" in error for error in errors)) + + +class AuditCommandTests(unittest.TestCase): + def test_audit_passes_on_the_shipped_curation_set(self): + self.assertEqual([], audit_curation.audit()) + + def test_audit_script_exits_zero(self): + completed = subprocess.run( + [sys.executable, str(AUDIT_SCRIPT)], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(0, completed.returncode, completed.stderr) + self.assertIn("curation audit passed", completed.stdout) + + def test_audit_script_reports_a_missing_records_directory(self): + completed = subprocess.run( + [sys.executable, str(AUDIT_SCRIPT), "--records", "/nonexistent/records"], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(1, completed.returncode) + self.assertIn("missing curation records directory", completed.stderr) + + def test_audit_fails_on_a_mutated_promotion(self): + with self.subTest("baseline is clean"): + self.assertEqual([], audit_curation.audit()) + mutated = copy.deepcopy( + _load(curation.DEFAULT_PROMOTIONS / "no-promotion-example.json") + ) + mutated["positive_case_ids"] = ["unpatched-token-scope-claim"] + tmp_dir = curation.DEFAULT_PROMOTIONS.parent / "promotions-mutated-tmp" + tmp_dir.mkdir(exist_ok=True) + self.addCleanup(tmp_dir.rmdir) + self.addCleanup(lambda: [p.unlink() for p in tmp_dir.glob("*.json")]) + (tmp_dir / "mutated.json").write_text(json.dumps(mutated)) + errors = audit_curation.audit(promotions_root=tmp_dir) + self.assertTrue(errors) + self.assertTrue( + any("unresolved claims cannot enter" in error for error in errors) + ) + + +if __name__ == "__main__": + unittest.main() From 07baa7dfdf06bfa19428bb9ba80a8317f8ff78d0 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 07:54:43 -0700 Subject: [PATCH 2/3] fix(review-suite): resolve a duplicate's disposition through its duplicate_of chain ## Summary - `_promotable_errors` now resolves a `duplicate` record's effective disposition by following `duplicate_of` (via new `_resolve_duplicate_disposition`, with cycle detection) before allowing it to fill a promotion decision's positive or negative slot. - Add two synthetic fixtures (`cache-ttl-duplicate-with-new-surface`, `unresolved-duplicate-claim`) and six regression tests covering: a distinct duplicate of an accepted record used as a negative control, of a rejected record used as a positive case, of an unresolved claim, a multi-hop duplicate chain, and a duplicate cycle. ## Why - Independent correctness review of this candidate found that a distinct duplicate's own disposition is always `duplicate` (never `accepted_*`/`rejected_*`), so the prior check let a duplicate of a rejected false positive support a positive case, or a duplicate of an accepted defect support a negative control - feeding a rejected non-finding into a rubric change as if it were a real defect, or the reverse. Resolving through the chain closes that gap without weakening the existing double-counting protection for a plain duplicate. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 3 + .../cache-ttl-duplicate-with-new-surface.json | 27 +++++++ .../records/unresolved-duplicate-claim.json | 27 +++++++ review-suite/scripts/evals/curation.py | 74 ++++++++++++++++- .../scripts/tests/test_eval_curation.py | 80 +++++++++++++++++++ 5 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 review-suite/evals/curation/records/cache-ttl-duplicate-with-new-surface.json create mode 100644 review-suite/evals/curation/records/unresolved-duplicate-claim.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 4142c78..bb0addb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,11 @@ summary: Chronological history of repository and skill changes. ## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, and added connector-outcome curation and promotion tooling +- fix(review-suite): resolve a duplicate's disposition through its duplicate_of + chain - feat(review-suite): add connector-outcome curation and promotion tooling, including the mechanical disclosure guardrail + (`d7357ee17a616ad374e6bb033a4c9adef6e5cc0a`) - docs: fix stale CHANGELOG SHAs left by the main rebase (`51cc734fc56a97dfa7a754fd046206dd62b375ba`) - docs: backfill the CHANGELOG entry for the review_gate.py canonicalization fix diff --git a/review-suite/evals/curation/records/cache-ttl-duplicate-with-new-surface.json b/review-suite/evals/curation/records/cache-ttl-duplicate-with-new-surface.json new file mode 100644 index 0000000..1e8c34d --- /dev/null +++ b/review-suite/evals/curation/records/cache-ttl-duplicate-with-new-surface.json @@ -0,0 +1,27 @@ +{ + "curation_record_version": "1.0", + "record_id": "cache-ttl-duplicate-with-new-surface", + "disposition": "duplicate", + "duplicate_of": "cache-ttl-false-positive", + "distinct_contribution": "Same conflated-cache misreading as cache-ttl-false-positive, but raised against the *admin-panel* cache constant rather than the client negative-result cache, giving a second, independent affected surface for the same non-finding.", + "public": { + "connector_finding": { + "claim": "A connector flagged, on an unrelated candidate, that the admin panel's cache TTL constant is too short.", + "connector_identity": "synthetic-connector-alpha" + }, + "contract_dimension": "correctness", + "affected_surface": "admin-panel cache TTL constant", + "adjudication_evidence": "Same non-finding as cache-ttl-false-positive (a short TTL on an intentionally short-lived client-local cache, conflated with an unrelated upstream cache), but raised against a distinct constant and surface, so it is retained as a second surface rather than folded away as a plain duplicate.", + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/evals/curation/records/unresolved-duplicate-claim.json b/review-suite/evals/curation/records/unresolved-duplicate-claim.json new file mode 100644 index 0000000..70c6e77 --- /dev/null +++ b/review-suite/evals/curation/records/unresolved-duplicate-claim.json @@ -0,0 +1,27 @@ +{ + "curation_record_version": "1.0", + "record_id": "unresolved-duplicate-claim", + "disposition": "duplicate", + "duplicate_of": "unpatched-token-scope-claim", + "distinct_contribution": "Same session-token scope concern, raised against a distinct refresh entry point, giving a second surface if the original claim ever resolves.", + "public": { + "connector_finding": { + "claim": "A connector flagged, on an unrelated candidate, that a second session-token refresh entry point may also request a broader scope.", + "connector_identity": "synthetic-connector-gamma" + }, + "contract_dimension": "correctness", + "affected_surface": "secondary session-token refresh entry point", + "adjudication_evidence": "Restates unpatched-token-scope-claim's still-unresolved concern on a distinct entry point. Adjudication of the underlying concern has not settled, so this record cannot be promoted either, regardless of its distinct surface.", + "provenance": { + "source_class": "synthetic", + "source_description": "hand-written synthetic fixture for this ticket's infrastructure proof" + } + }, + "private": { + "provenance": { + "retention_authority": "repository owner, synthetic-fixture scope only", + "owner": "review-suite maintainers", + "review_date": "2026-07-29" + } + } +} diff --git a/review-suite/scripts/evals/curation.py b/review-suite/scripts/evals/curation.py index 344586b..f44733f 100644 --- a/review-suite/scripts/evals/curation.py +++ b/review-suite/scripts/evals/curation.py @@ -288,15 +288,53 @@ def load_records(root: Path | None = None) -> RecordSet: return RecordSet(root=root, records=records) +def _resolve_duplicate_disposition( + record_id: str, + records: dict[str, dict[str, Any]], + *, + _seen: frozenset[str] = frozenset(), +) -> tuple[str | None, str | None]: + """Follow `duplicate_of` to the disposition a duplicate's role must match. + + A `duplicate` record carries no `expected_root_cause` or + `accepted_non_finding` of its own - the schema forbids both - so whether it + may fill a positive or negative promotion slot depends entirely on the + disposition of the record it actually restates, followed through however + many `duplicate` hops it takes to reach one. Returns `(None, None)` when + the chain is missing a record or cycles back on itself; callers must treat + that as a validation error rather than silently permitting the duplicate. + """ + record = records.get(record_id) + if record is None or record_id in _seen: + return None, None + disposition = record.get("disposition") + if disposition != "duplicate": + return record_id, disposition + duplicate_of = record.get("duplicate_of") + if not duplicate_of: + return None, None + return _resolve_duplicate_disposition( + duplicate_of, records, _seen=_seen | {record_id} + ) + + def _promotable_errors( - record_id: str, record: dict[str, Any], *, as_positive: bool + record_id: str, + record: dict[str, Any], + records: dict[str, dict[str, Any]], + *, + as_positive: bool, ) -> list[str]: """Whether one record may support a positive or negative promotion slot. Unresolved claims can never enter grading expectations or modify active review guidance. A duplicate without a recorded distinct contribution can never be promoted either - promoting it would double-count the root cause - it shares with the record it duplicates. + it shares with the record it duplicates. A duplicate *with* a distinct + contribution may only fill the positive/negative role its resolved + root-cause record actually supports - a distinct duplicate of a rejected + claim is still evidence the claim was rejected, not a second accepted + outcome, and vice versa. """ disposition = record.get("disposition") if disposition == "unresolved": @@ -311,6 +349,32 @@ def _promotable_errors( "negative control cannot be promoted without double-counting its root " "cause" ] + resolved_id, resolved_disposition = _resolve_duplicate_disposition( + record_id, records + ) + if resolved_disposition is None: + return [ + f"{record_id}: duplicate_of chain could not be resolved to a " + "non-duplicate disposition (a missing record or a cycle)" + ] + if resolved_disposition == "unresolved": + return [ + f"{record_id}: duplicates an unresolved claim ({resolved_id!r}), which " + "cannot enter grading expectations or modify active review guidance " + "either" + ] + if as_positive and resolved_disposition not in ACCEPTED_DISPOSITIONS: + return [ + f"{record_id}: duplicates {resolved_id!r} whose disposition " + f"{resolved_disposition!r} is not an accepted material outcome, so it " + "cannot support a positive regression case" + ] + if not as_positive and resolved_disposition not in REJECTED_TUNING_DISPOSITIONS: + return [ + f"{record_id}: duplicates {resolved_id!r} whose disposition " + f"{resolved_disposition!r} is not rejected/deferred tuning evidence, " + "so it cannot support a negative control" + ] return [] if as_positive and disposition not in ACCEPTED_DISPOSITIONS: return [ @@ -380,14 +444,16 @@ def validate_promotion_decision( errors.append(f"positive_case_ids: unknown record {record_id!r}") continue errors.extend( - _promotable_errors(record_id, records[record_id], as_positive=True) + _promotable_errors(record_id, records[record_id], records, as_positive=True) ) for record_id in negative_ids: if record_id not in records: errors.append(f"negative_control_case_ids: unknown record {record_id!r}") continue errors.extend( - _promotable_errors(record_id, records[record_id], as_positive=False) + _promotable_errors( + record_id, records[record_id], records, as_positive=False + ) ) overlap = sorted(set(positive_ids) & set(negative_ids)) if overlap: diff --git a/review-suite/scripts/tests/test_eval_curation.py b/review-suite/scripts/tests/test_eval_curation.py index 9676238..5b7019b 100644 --- a/review-suite/scripts/tests/test_eval_curation.py +++ b/review-suite/scripts/tests/test_eval_curation.py @@ -214,6 +214,86 @@ def test_a_distinct_duplicate_may_be_promoted(self): [], curation.validate_promotion_decision(document, self.records) ) + def test_a_distinct_duplicate_of_an_accepted_record_cannot_be_a_negative_control( + self, + ): + """A duplicate's promotion role must match what it actually duplicates. + + `retry-jitter-duplicate-with-new-surface` duplicates + `retry-jitter-defect`, whose disposition is `accepted_acceptance_miss`. + Restating an accepted defect on a second surface is still evidence the + defect was accepted, never evidence it was rejected, so it must not be + usable as a negative control even though it declares a distinct + contribution. + """ + document = self._promotion("no-promotion-example") + document["negative_control_case_ids"] = [ + "retry-jitter-duplicate-with-new-surface" + ] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("cannot support a negative control" in error for error in errors) + ) + + def test_a_distinct_duplicate_of_a_rejected_record_cannot_be_a_positive_case(self): + """The symmetric case: a duplicate of a rejected false positive. + + `cache-ttl-duplicate-with-new-surface` duplicates + `cache-ttl-false-positive` (`rejected_false_positive`) on a distinct + surface. It must not be usable as a positive case even with a distinct + contribution declared, or a rejected non-finding could feed a global + rubric or repository-instruction change as if it were a real defect. + """ + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["cache-ttl-duplicate-with-new-surface"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any( + "cannot support a positive regression case" in error for error in errors + ) + ) + + def test_a_distinct_duplicate_of_a_rejected_record_may_be_a_negative_control(self): + document = self._promotion("no-promotion-example") + document["negative_control_case_ids"] = ["cache-ttl-duplicate-with-new-surface"] + self.assertEqual( + [], curation.validate_promotion_decision(document, self.records) + ) + + def test_a_duplicate_of_an_unresolved_claim_cannot_be_promoted_either(self): + document = self._promotion("no-promotion-example") + document["negative_control_case_ids"] = ["unresolved-duplicate-claim"] + errors = curation.validate_promotion_decision(document, self.records) + self.assertTrue( + any("duplicates an unresolved claim" in error for error in errors) + ) + + def test_a_duplicate_chain_resolves_through_more_than_one_hop(self): + """`_resolve_duplicate_disposition` must follow a duplicate-of-a-duplicate.""" + chained = copy.deepcopy(self.records["retry-jitter-duplicate-with-new-surface"]) + chained["record_id"] = "retry-jitter-duplicate-chain" + chained["duplicate_of"] = "retry-jitter-duplicate-with-new-surface" + chained["distinct_contribution"] = "A third surface, one more hop away." + records = {**self.records, "retry-jitter-duplicate-chain": chained} + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["retry-jitter-duplicate-chain"] + document["negative_control_case_ids"] = [] + self.assertEqual([], curation.validate_promotion_decision(document, records)) + + def test_a_duplicate_cycle_cannot_be_resolved_and_is_rejected(self): + first = copy.deepcopy(self.records["retry-jitter-duplicate-with-new-surface"]) + first["record_id"] = "cycle-a" + first["duplicate_of"] = "cycle-b" + second = copy.deepcopy(first) + second["record_id"] = "cycle-b" + second["duplicate_of"] = "cycle-a" + records = {**self.records, "cycle-a": first, "cycle-b": second} + document = self._promotion("no-promotion-example") + document["positive_case_ids"] = ["cycle-a"] + document["negative_control_case_ids"] = [] + errors = curation.validate_promotion_decision(document, records) + self.assertTrue(any("could not be resolved" in error for error in errors)) + def test_a_rejected_record_cannot_support_a_positive_case(self): document = self._promotion("no-promotion-example") document["positive_case_ids"] = ["cache-ttl-false-positive"] From 45d5930d567dfa698bc25a4261a1959640de72d4 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Wed, 29 Jul 2026 08:27:15 -0700 Subject: [PATCH 3/3] refactor(review-suite): simplify duplicate-chain resolution and unify its membership check ## Summary - `_resolve_duplicate_disposition` is now an iterative loop with a local `seen` set instead of self-recursion carrying a `_seen` frozenset accumulator parameter. Same behavior (including cycle detection), simpler control flow. - `_promotable_errors` now computes one `(effective_id, effective_disposition)` pair after the duplicate-only early returns, then runs the accepted/rejected disposition-membership check exactly once, selecting the existing error-message wording only on whether `effective_id == record_id`. Previously the same membership rule was checked twice: once for a duplicate's resolved disposition, once for a direct record's own disposition. - No behavior change: all 45 existing curation tests pass unmodified, including the multi-hop chain and cycle-detection cases. ## Why - Code-simplicity review of this candidate flagged both as unnecessary local complexity: recursion for a simple bounded linear traversal, and duplicated membership-check logic that could drift out of sync between its two copies. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 4 +- review-suite/scripts/evals/curation.py | 87 ++++++++++++++------------ 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb0addb..d26b9e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,8 +6,10 @@ summary: Chronological history of repository and skill changes. ## 2026-07-29 — Migrated implement-ticket and babysit-pr to consume the final review-result contract, rechecked the s2/s3 strata under grader 1.1 for the same surface-in-prose defect, and added connector-outcome curation and promotion tooling +- refactor(review-suite): simplify duplicate-chain resolution and unify its + membership check - fix(review-suite): resolve a duplicate's disposition through its duplicate_of - chain + chain (`07baa7dfdf06bfa19428bb9ba80a8317f8ff78d0`) - feat(review-suite): add connector-outcome curation and promotion tooling, including the mechanical disclosure guardrail (`d7357ee17a616ad374e6bb033a4c9adef6e5cc0a`) diff --git a/review-suite/scripts/evals/curation.py b/review-suite/scripts/evals/curation.py index f44733f..26e5b42 100644 --- a/review-suite/scripts/evals/curation.py +++ b/review-suite/scripts/evals/curation.py @@ -289,10 +289,7 @@ def load_records(root: Path | None = None) -> RecordSet: def _resolve_duplicate_disposition( - record_id: str, - records: dict[str, dict[str, Any]], - *, - _seen: frozenset[str] = frozenset(), + record_id: str, records: dict[str, dict[str, Any]] ) -> tuple[str | None, str | None]: """Follow `duplicate_of` to the disposition a duplicate's role must match. @@ -304,18 +301,20 @@ def _resolve_duplicate_disposition( the chain is missing a record or cycles back on itself; callers must treat that as a validation error rather than silently permitting the duplicate. """ - record = records.get(record_id) - if record is None or record_id in _seen: - return None, None - disposition = record.get("disposition") - if disposition != "duplicate": - return record_id, disposition - duplicate_of = record.get("duplicate_of") - if not duplicate_of: - return None, None - return _resolve_duplicate_disposition( - duplicate_of, records, _seen=_seen | {record_id} - ) + seen: set[str] = set() + current = record_id + while True: + record = records.get(current) + if record is None or current in seen: + return None, None + disposition = record.get("disposition") + if disposition != "duplicate": + return current, disposition + seen.add(current) + duplicate_of = record.get("duplicate_of") + if not duplicate_of: + return None, None + current = duplicate_of def _promotable_errors( @@ -334,7 +333,10 @@ def _promotable_errors( contribution may only fill the positive/negative role its resolved root-cause record actually supports - a distinct duplicate of a rejected claim is still evidence the claim was rejected, not a second accepted - outcome, and vice versa. + outcome, and vice versa. Once resolved, a direct record and a resolved + duplicate share exactly one disposition-membership check below; only the + error message's phrasing differs, selected by whether the resolved id is + the record's own id. """ disposition = record.get("disposition") if disposition == "unresolved": @@ -342,6 +344,7 @@ def _promotable_errors( f"{record_id}: unresolved claims cannot enter grading expectations or " "modify active review guidance" ] + if disposition == "duplicate": if not record.get("distinct_contribution"): return [ @@ -349,42 +352,46 @@ def _promotable_errors( "negative control cannot be promoted without double-counting its root " "cause" ] - resolved_id, resolved_disposition = _resolve_duplicate_disposition( + effective_id, effective_disposition = _resolve_duplicate_disposition( record_id, records ) - if resolved_disposition is None: + if effective_disposition is None: return [ f"{record_id}: duplicate_of chain could not be resolved to a " "non-duplicate disposition (a missing record or a cycle)" ] - if resolved_disposition == "unresolved": + if effective_disposition == "unresolved": return [ - f"{record_id}: duplicates an unresolved claim ({resolved_id!r}), which " - "cannot enter grading expectations or modify active review guidance " - "either" + f"{record_id}: duplicates an unresolved claim ({effective_id!r}), " + "which cannot enter grading expectations or modify active review " + "guidance either" ] - if as_positive and resolved_disposition not in ACCEPTED_DISPOSITIONS: - return [ - f"{record_id}: duplicates {resolved_id!r} whose disposition " - f"{resolved_disposition!r} is not an accepted material outcome, so it " - "cannot support a positive regression case" - ] - if not as_positive and resolved_disposition not in REJECTED_TUNING_DISPOSITIONS: + else: + effective_id, effective_disposition = record_id, disposition + + if as_positive and effective_disposition not in ACCEPTED_DISPOSITIONS: + if effective_id == record_id: return [ - f"{record_id}: duplicates {resolved_id!r} whose disposition " - f"{resolved_disposition!r} is not rejected/deferred tuning evidence, " - "so it cannot support a negative control" + f"{record_id}: disposition {effective_disposition!r} is not an " + "accepted material outcome and cannot support a positive regression " + "case" ] - return [] - if as_positive and disposition not in ACCEPTED_DISPOSITIONS: return [ - f"{record_id}: disposition {disposition!r} is not an accepted material " - "outcome and cannot support a positive regression case" + f"{record_id}: duplicates {effective_id!r} whose disposition " + f"{effective_disposition!r} is not an accepted material outcome, so it " + "cannot support a positive regression case" ] - if not as_positive and disposition not in REJECTED_TUNING_DISPOSITIONS: + if not as_positive and effective_disposition not in REJECTED_TUNING_DISPOSITIONS: + if effective_id == record_id: + return [ + f"{record_id}: disposition {effective_disposition!r} is not " + "rejected/deferred tuning evidence and cannot support a negative " + "control" + ] return [ - f"{record_id}: disposition {disposition!r} is not rejected/deferred " - "tuning evidence and cannot support a negative control" + f"{record_id}: duplicates {effective_id!r} whose disposition " + f"{effective_disposition!r} is not rejected/deferred tuning evidence, " + "so it cannot support a negative control" ] return []