feat(reporting): record the evaluated source identity on benchmark cards - #131
Open
MaxFreedomPollard wants to merge 1 commit into
Open
Conversation
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>
MaxFreedomPollard
requested review from
chrisknvidia,
mosheabr,
rmalani-nv and
rng1995
as code owners
September 4, 2026 11:47
Collaborator
|
@mosheabr : What do you think about this Benchmark changes? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
A published
BENCHMARK.mdrecords 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 was0117bc2e…— 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:
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:
evaluated_sourceargument tobuild_agent_eval_payload/agent_eval_result_from_directory;evaluated_sourceobject in the run'srun_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
PASSwithout a completed Tier 3 run (Tier 1-only, or an advisory Tier 3 skip undertier3_required = false). Those shapes have noagent_evalpayload 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_policyalready 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.pyis a small dependency-free module holding the field patterns. It sits alongsideevidence.pyandspdx.pyrather than underevaluation/orreporting/, becauseevaluationalready importsreportingand 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 corruptedholgerroth/nvflare_examplesandghcr.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, sototally-fake:0000…cannot read as a canonical digest.revision, notcommit. The issue accepts "commit SHA, or a canonical digest of the evaluated skill content". A line labelledcommitcarrying asha256:digest would be its own small lie, so the line readsEvaluated 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 existinglegacy or non-live resultwording, 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-provenancerequires the three fields and fails any card publishing aPASSwithout 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.pyis 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:
output_provenance.pycovers atomic writes, not content signatures), so this belongs downstream.Git commitare renamed or regenerated" — the renderer onmainalready stopped emitting the ambiguousGit commitfield, as the issue notes. Regenerating already-published downstream cards is outside this repo.No in-tree producer populates the identity yet. Nothing in
src/writesevaluated_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 rendernot recordedand the default gate accepts them. I'm happy to add a CLI option or thread it into the Harbor runner'srun_configin 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.pyadds 67 tests covering normalization, cross-channel resolution and conflict detection, the payload contract, rendering, the non-Tier-3 carrier, both gate modes, themain()/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 recordedrather 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 passedscripts/ci/check_public_benchmarks.py --require-files tests/golden— passes (the exact CI invocation)--require-source-provenance— passesbenchmark_pass/benchmark_failregenerated; the fixture deliberately usesNVIDIA/demo_skillsand a digest-pinned container reference, so the two previously-corrupted characters are covered by the golden itselfOne note in passing, unrelated to this change:
make lintrunsruff check src testsand reports 54 pre-existing errors on a cleanmain, while CI runsruff check .and is green. The Makefile target may want to match CI.Closes #72