docs: harden datastore PR review workflow - #241
Conversation
f595b3e to
1dbac8d
Compare
nelaturuharsha
changed the title
Reviewed against the schema, A patch for the mechanical parts is at the bottom of this comment — duplication with Decisions this needs before it can land1. Do review artifacts go into Our recommendation: don't. Provenance for a value already has the right home, and this 2. Which fields the gate covers. The highest-value call here. The gate as written Suggested boundary — gate join keys and closed vocabularies, not open annotation:
This affects contributors — your submitted id won't be silently rewritten — so it wants a 3. One validator check, and it isn't the one I first suggested. I scanned all 13,228 Non-finite bounds — withdrawn, already covered. Path vs Two narrow variants have measured precision instead — 22 records on
And the #151 defect itself is a diff-level invariant, not a record-level one: if Byproducts of the scan, worth fixing as data rather than code: one off-layout record on 4. A scale threshold for the gate. #164 is 189 model directories; #158 adds 987 5. Cleanup of what already merged. Suggestions on your design (yours to take or leave)
Notes, not blockers
Patch for the mechanical parts (applies on
|
mrshu
left a comment
mrshu
left a comment
There was a problem hiding this comment.
⚒️ review-anvil report
Review decision: COMMENT — The new approval and evidence controls are a strong direction; three medium workflow edges would benefit from clarification before agents rely on the durable receipts.
Result: 3 medium findings, 1 low suggestion, and 1 item set aside after its verification protocol did not complete.
Scope: Five datastore-review skill and template files at 5f861a59bc33690e0640fcd538fe031b3c6afcf5 (+364/−15).
Checks: Three Codex reviewers examined the exact head. The three actionable concerns had reviewer consensus and did not require reproduction; the single-reviewer scope concern was set aside after two protocol-invalid verifier results.
Second check: Two targeted Codex adversaries challenged approval and audit lifecycle behavior. They reinforced the reported concerns; adversarial-only ideas were not promoted into findings.
Findings
RAV-RUN1-R1-F001 [medium] audit-closure — the append-only record has no defined way to close a pending bot receipt
The decision entry is created before upload, while the matching bot run exists only afterward. The template permits pending, but the later monitoring steps do not require an append-only closure receipt. A small receipt-only entry linked to the original decision and validated data head would keep the durable record complete without rewriting history.
RAV-RUN1-R1-F002 [medium] evidence-resolution — durable decisions can contain source and approval references that cannot be resolved
Decision rows cite source IDs, but the template has no Sources section when the deployment proposal is n/a. The approval field also records a person, time, and head without a stable event URL or receipt ID. A compact Sources section and approval-receipt field would make these records independently traceable.
RAV-RUN1-R1-F003 [medium] authorization-scope — approval is undefined for non-deployment metadata backfills
The missingness protocol covers temperature, token limits, and similar fields and requires approval for available_not_surfaced. The concrete digest-and-head approval flow covers only deployment axes, so an agent cannot tell whether ordinary fix authorization or a new artifact governs other backfills. Defining that boundary would preserve the PR's careful mutation controls.
1 low-priority suggestion
- RAV-RUN1-R1-F005 [low] evidence-threshold — The proposal row permits categorical values beside
lowconfidence even though the references allow categorical proposals only at high or medium confidence. Encoding that condition in the template would make the safe combination easier to follow.
Set aside / Outside this change
- RAV-RUN1-R1-F004 [medium] approval-scope: One reviewer noted that ordinary proposal rows are not mapped to exact paths or evaluation IDs. We set this aside because both verifier attempts returned protocol-invalid status, so the required independent check did not complete.
Run details
- Target: PR #241, head
5f861a59bc33690e0640fcd538fe031b3c6afcf5 - Rounds: 1/1 completed; adaptive continuation not needed
- Mix: 3
codex-execreviewers; Codex-only verification and adversarial checks - Adversarial mode: auto, targeted
- Approval policy: never; comment-only
- Reviewer timeout: 1,200 seconds
- Focus: constructive, positive review language and optional suggestions while preserving technical accuracy
- Earlier review-anvil history: none detected by the helper
Reviewed with review-anvil.
| - Proposal SHA-256: `<full digest or n/a>` | ||
| - Approval: `<approver, timestamp, and exact approved head or n/a>` | ||
| - Local verification: `<validator and duplicate-check receipts>` | ||
| - Matching bot receipt: `<event/run/fingerprint or pending>` |
There was a problem hiding this comment.
RAV-RUN1-R1-F001 [medium] audit-closure — The append-only record can remain at pending after validation completes. This entry must exist before upload, while the matching bot run exists only afterward, and the later workflow does not define a closure entry. A receipt-only append linked to this decision and the validated data head would preserve both completeness and append-only history.
|
|
||
| ### Decisions | ||
|
|
||
| - `<field or issue>`: `<source-backed old → new value and concise reason>` (`<source ids>`) |
There was a problem hiding this comment.
RAV-RUN1-R1-F002 [medium] evidence-resolution — These source IDs have no resolver when Approved proposal is n/a, and the approval field above has no durable event URL or receipt ID. A compact per-entry Sources section plus an approval-receipt field would let future readers trace both the evidence and authorization.
| |---|---|---| | ||
| | `record_absent` | The submitted record omits the field; source research has not finished | Say "not surfaced in the submitted record" and investigate | | ||
| | `research_incomplete` | One or more relevant primary surfaces or exact identities remain unchecked | Continue research; do not propose `unknown` or claim source missingness | | ||
| | `available_not_surfaced` | A reliable source contains the value but the record does not | Backfill with approval and fix/follow up on the adapter or submission path | |
There was a problem hiding this comment.
RAV-RUN1-R1-F003 [medium] authorization-scope — This approval requirement also applies to temperature, token limits, and other non-deployment fields, but the skill defines a head-bound artifact only for deployment axes. Stating whether ordinary fix authorization or a lightweight head-bound receipt governs these backfills would keep the mutation boundary unambiguous.
|
|
||
| For `model_availability`, inspect the exact artifact, not only a family README. A | ||
| downloadable gated or restrictively licensed checkpoint is `open_weights` under the | ||
| current schema. API access alone is `closed_weights`. Treat delta-only releases, |
There was a problem hiding this comment.
How are we defining API access here? The current phrasing seems to point to hosted open weight models as closed weight. It seems this is a registry-level decision and registry would classify this as open weight and make the API distinction in deployment_type?
| an absent field genuinely missing while its status is `record_absent` or | ||
| `research_incomplete`. If a README, eval card, methodology page, leaderboard, paper, | ||
| repository, or API exposes it, classify it as `available_not_surfaced` and identify | ||
| the adapter/submission gap. |
There was a problem hiding this comment.
Nit: do we mean model card or similar artifacts when we say "eval card" in these files
| submitted run's value. A harness default is evidence only when the pinned version and | ||
| run configuration establish that the default governed this run. |
There was a problem hiding this comment.
I think it would be helpful to operationalize this further. Terms such as “exact model,” “concrete lead,” and “the default governed this run” leave substantial judgment to the agent. Could we add a few qualifying and non-qualifying examples?
For instance:
- Qualifies:
- The source leaderboard, methodology, or result page directly associates the value with the submitted result.
- The evaluation README or paper says that all results in a defined release, table, or run group used the value.
- A run-specific source says that a named, pinned library’s defaults were used, and that version documents the value.
- A configuration or run script tied to the submitted results sets the value, with no submitted configuration showing an override.
- Does not qualify:
- A snippet from a search engine or tool; open and inspect the underlying source instead.
- A generic model-family, provider, or library default without evidence that it applied to this evaluation.
- A current library default when the run’s version is unknown.
- An example configuration or run script that is not tied to the submitted results.
|
|
||
| Use this protocol before claiming that any reproducibility or model field is missing. | ||
| It applies to deployment metadata, temperature, token limits, and other fields that an | ||
| adapter or submitter may have failed to surface. |
There was a problem hiding this comment.
Consider clarifying with:
Evidence should also be assessed per field: a source may establish one missing value without establishing every missing field in the record.
4 participants
What / source
Harden the EEE datastore PR-review skill based on live reviews of Hugging Face datastore PRs 129, 145, 151, and 164.
The skill now emits mandatory incremental receipts and one-minute heartbeats, binds deployment-metadata approval to both a proposal SHA-256 and the exact datastore PR head, and distinguishes record omissions from metadata that remains unavailable after a documented first-party search.
PR 164 exposed two further cases now covered here. Citation-backed aggregators do not establish the deployment used by every evaluation they collect: the reviewer must follow per-result provenance, must not infer a deployment difference from generation parameters or multiple citations alone, and must not flatten proven conflicting runs to one model-wide value. Score repairs must restore the source value to the metric's declared scale and preserve the conversion instead of widening bounds; bare non-finite JSON bounds are serialized as the schema-supported strings without changing their meaning.
Approved decisions are also preserved with the collection. The byte-identical approved proposal is copied to
data/<collection>/deployment-metadata-proposal-<digest-prefix>.md, and an append-onlydata/<collection>/REVIEW_DECISIONS.mdentry records the PR/head, approval receipt, substantive decisions, evidence, validation, and follow-ups. This gives each collection durable provenance and a changelog rather than leaving the audit trail only in temporary notes or discussion history.Follow-up to #240.
Review lane
Design agreed in live operator feedback while forward-testing the skill on
evaleval/EEE_datastorediscussions 129, 145, 151, and 164.Checklist
REVIEW_DECISIONS.mdis append-only and records approval, evidence, validation, and follow-upsInfinityvalues are repaired without changing unbounded semantics.gitignorechange excludedDecisions & coverage
Decision / where: progress checkpoints in the core workflow
Chose / instead of: independently verifiable phase receipts with bounded polling / final-only reporting
Confidence (high/med/low): high
General? (yes/no): yes
Decision / where: deployment research approval artifact
Chose / instead of: a compact Markdown table with source IDs and a head-bound digest / verbose JSON or implicit approval from a general fix request
Confidence (high/med/low): high
General? (yes/no): yes
Decision / where: collection provenance
Chose / instead of: byte-identical approved proposal plus append-only collection decision log / temporary run notes or discussion-only history
Confidence (high/med/low): high
General? (yes/no): yes
Decision / where: aggregator deployment metadata
Chose / instead of: follow each result to run-level evidence and group only evidence-equivalent files / infer deployment from the aggregator, model id, citation count, or generation settings
Confidence (high/med/low): high
General? (yes/no): yes
Decision / where: out-of-range source scores
Chose / instead of: source-backed conversion to the declared metric scale with raw value and math retained / widening bounds or clamping the score
Confidence (high/med/low): high
General? (yes/no): yes
Decision / where: missing reproducibility metadata
Chose / instead of: separate
record_absent,research_incomplete,available_not_surfaced,conflicting_sources, andunavailable_after_searchstatuses / treating an adapter omission as a claim about the underlying evaluationConfidence (high/med/low): high
General? (yes/no): yes
Decision / where: low-confidence deployment/availability classification
Chose / instead of: continue research, then propose
unknownonly after the stop rule / provider-, family-, or default-based guessingConfidence (high/med/low): high
General? (yes/no): yes
Coverage: six workflow checkpoints, one head-bound approval artifact, collection-root proposal and decision-log templates, a six-surface missingness ledger, aggregator/run grouping rules, source-scale repair guardrails, axis-specific confidence/source shorthand, and explicit adapter/registry follow-ups; no source rows or EEE records changed.
Operator asked about policy calls? Yes. The operator chose explicit human approval before applying researched deployment metadata, durable collection-root provenance, and a bounded evidence standard before describing metadata as unavailable.
Verification
Skill is valid!git diff --check