Skip to content

feat(reporting): record the evaluated source identity on benchmark cards - #131

Open
MaxFreedomPollard wants to merge 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:fix/issue-72-evaluated-source-provenance
Open

feat(reporting): record the evaluated source identity on benchmark cards#131
MaxFreedomPollard wants to merge 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:fix/issue-72-evaluated-source-provenance

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown

What this changes

A published BENCHMARK.md records the evaluator build but not the source tree it evaluated. As #72 shows, three skills from three different repositories published cards whose only recorded revision was 0117bc2e… — the SkillEvaluator container tag they happened to share. A reviewer reading a committed card cannot tell which version of a skill was evaluated, or whether the card went stale after the source changed.

This carries an evaluated-source identity through the result contract and renders it with labels that are unambiguously distinct from the evaluator provenance:

- Evaluator version: `0.8.2`
- Evaluated source: `NVIDIA/NVFlare`
- Evaluated source revision: `2263a2ebdab903e87f7e7c0a001d22c3a926a9cf`
- Evaluator container revision: `ghcr.io/nvidia/skillevaluator@sha256:0117bc2e…`

Design notes

The identity comes from the orchestration input, never from repository state. Per the issue's requirement, nothing is inferred at render time — the tree that renders a card is the evaluator checkout, not the evaluated source, so inferring it there would reproduce the bug being fixed. Three seams supply it:

  • the evaluated_source argument to build_agent_eval_payload / agent_eval_result_from_directory;
  • an evaluated_source object in the run's run_config.json (SkillEvaluator writes that file itself, so a pipeline using this route must add the key to the generated run artifact before the card renders);
  • metadata["evaluated_source"] on any validation result.

That last seam matters more than it looks: a card can publish a PASS without a completed Tier 3 run (Tier 1-only, or an advisory Tier 3 skip under tier3_required = false). Those shapes have no agent_eval payload to carry an identity, so without a result-level carrier the strict gate below would be permanently unsatisfiable for any catalog containing one. The renderer resolves the identity the same way _benchmark_policy already resolves the persisted policy.

Conflicting inputs fail rather than resolving by precedence. Acceptance criterion 3 asks publication to fail closed when provenance "is missing or conflicts with the orchestration input". Fields merge individually across the two channels — so a typo in one cannot silently discard a good identity in the other — but a field both channels supply with different values raises EvaluatedSourceConflict. A card that quietly picked one of two contradictory revisions would be exactly the unverifiable provenance this contract exists to prevent. No in-tree caller supplies either channel today, so this cannot fire on existing behaviour.

One definition, shared by producer, renderer and gate. skillevaluator/source_identity.py is a small dependency-free module holding the field patterns. It sits alongside evidence.py and spdx.py rather than under evaluation/ or reporting/, because evaluation already imports reporting and the renderer needs it too. (The CI script mirrors the shapes as literals rather than importing, since it is deliberately stdlib-only.)

Validated values are published verbatim, and that is deliberate. An earlier draft ran the identity through _publication_safe_inline. That escaping rewrites _ to \_ and @ to @, which silently corrupted holgerroth/nvflare_examples and ghcr.io/…@sha256:… — turning the immutable, digest-pinned reference the issue asks for into a string resolving to nothing, which this patch's own gate then rejected. Validation is what makes a value safe, so the card emits the canonical value unchanged. Every accepted character is inert inside a code span, and the renderer re-validates rather than trusting the payload, so a card rendered from a hand-built or legacy metadata dict drops a hostile value instead of escaping it into the output. Digest algorithms are an allowlist (sha256/sha384/sha512), not a charset, so totally-fake:0000… cannot read as a canonical digest.

revision, not commit. The issue accepts "commit SHA, or a canonical digest of the evaluated skill content". A line labelled commit carrying a sha256: digest would be its own small lie, so the line reads Evaluated source revision; a commit takes precedence when both are supplied.

An absent identity says why. These fields fall back to not recorded (not supplied by the orchestration input) rather than borrowing the existing legacy or non-live result wording, which would be false — a fresh live run with no orchestration input is neither legacy nor non-live.

The gate check is entirely opt-in. --require-source-provenance requires the three fields and fails any card publishing a PASS without a recorded repository and revision — whatever the Tier 3 row says, and whether or not the verdict line is blockquoted (the rollout runbook points this flag at hand-authored backfill trees). Without the flag the scan is byte-compatible with its previous behaviour: cards generated before this contract existed cannot carry these fields. tests/test_public_benchmark_gate.py is untouched by this diff, which is the evidence that default behaviour did not move.

If you would rather it fail closed by default, say the word and I will flip it and update the fixtures here.

Scope

Two of the issue's acceptance criteria are deliberately not addressed:

  • "Signed output covers the recorded source identity" — there is no card-signing path in this repository (output_provenance.py covers atomic writes, not content signatures), so this belongs downstream.
  • "Existing cards using Git commit are renamed or regenerated" — the renderer on main already stopped emitting the ambiguous Git commit field, as the issue notes. Regenerating already-published downstream cards is outside this repo.

No in-tree producer populates the identity yet. Nothing in src/ writes evaluated_source; it arrives from the orchestration input, matching your note that "part of the fix may belong in the downstream publication pipeline". Until that side supplies it, cards render not recorded and the default gate accepts them. I'm happy to add a CLI option or thread it into the Harbor runner's run_config in this PR if you would prefer the producer in-tree — I left it out because the shape of that input is your call.

I also left _tier3_evidence_complete() alone: making the "Recommended for publication" decision depend on source provenance is a policy change with far wider blast radius than the CI gate.

Tests

tests/test_benchmark_source_provenance.py adds 67 tests covering normalization, cross-channel resolution and conflict detection, the payload contract, rendering, the non-Tier-3 carrier, both gate modes, the main()/find_offenders() entry points, and a reporter→gate round-trip parametrized over the values that previously broke (_ in a repository name, an @sha256: container reference, a content digest).

It includes the regression the issue names — "two source repositories evaluated with the same evaluator image retain different source SHAs" — and asserts the stronger property that actually matters: neither card's source revision ever equals the shared container revision, which is the exact confusion #72 reported. A companion test covers a run with no commit, checking the card says not recorded rather than borrowing the evaluator's revision.

Hostile-payload tests confirm that markdown, HTML and line-injection attempts through an unvalidated metadata dict are dropped rather than escaped into the card.

Verification

  • uv run python -m pytest -q — 5901 passed, 18 skipped (baseline before this change: 5834 passed)
  • uv run ruff check . — all checks passed
  • scripts/ci/check_public_benchmarks.py --require-files tests/golden — passes (the exact CI invocation)
  • The same gate with --require-source-provenance — passes
  • A pre-patch card scanned with default flags — passes, unchanged
  • Golden benchmark_pass / benchmark_fail regenerated; the fixture deliberately uses NVIDIA/demo_skills and a digest-pinned container reference, so the two previously-corrupted characters are covered by the golden itself

One note in passing, unrelated to this change: make lint runs ruff check src tests and reports 54 pre-existing errors on a clean main, while CI runs ruff check . and is green. The Makefile target may want to match CI.

Closes #72

A published BENCHMARK.md recorded the evaluator build but not the source
tree it evaluated. Two skills evaluated from different repositories by the
same evaluator container therefore produced cards whose only revision was
the shared container tag, so a reviewer could not tell which version of a
skill was evaluated or whether a card had gone stale.

Carry an evaluated-source identity through the result contract and render
it with labels unambiguously distinct from the evaluator provenance:

- Evaluated source: `NVIDIA/NVFlare`
- Evaluated source revision: `2263a2eb...`
- Evaluator container revision: `ghcr.io/nvidia/skillevaluator@sha256:...`

The identity comes from the orchestration input and is never inferred from
repository state while rendering: the tree that renders a card is the
evaluator checkout, not the evaluated source. It can arrive as the
`evaluated_source` argument, as `evaluated_source` in `run_config.json`, or
as `metadata["evaluated_source"]` on any validation result -- the last so a
card that publishes without a completed Tier 3 run still has a carrier.
Where two channels supply the same field and disagree, resolution fails
rather than silently picking one, because a card that quietly chose between
two contradictory revisions is the unverifiable provenance this contract
exists to prevent.

`skillevaluator.source_identity` holds one definition of the identity so
producer, renderer and gate agree. Values are accepted only in canonical
shape and are then published verbatim rather than Markdown-escaped, because
escaping rewrites `_` to `\_` and `@` to `&NVIDIA#64;` and would publish a
repository name and a digest-pinned image reference that no longer resolve.
Every accepted character is inert inside a code span, and the renderer
re-validates rather than trusting the payload, so a card built from a
hand-made or legacy metadata dict drops a hostile value instead of escaping
it into the output.

The revision line is labelled `revision` rather than `commit` because the
issue accepts either a commit SHA or a canonical content digest, and an
absent identity says it was not supplied by the orchestration input rather
than blaming a legacy or non-live result.

`check_public_benchmarks.py --require-source-provenance` requires the three
fields and fails any card publishing a PASS without a recorded repository
and revision, whether or not that verdict is blockquoted and whatever the
Tier 3 row says. The whole check is opt-in: cards generated before this
contract existed cannot satisfy it, so the default scan stays byte
compatible with its previous behaviour and CI can enable the flag once the
orchestration side supplies the identity.

Closes NVIDIA#72

Signed-off-by: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com>
@chrisknvidia

Copy link
Copy Markdown
Collaborator

@mosheabr : What do you think about this Benchmark changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Published BENCHMARK.md does not identify the evaluated source revision

2 participants