Skip to content

fix(reporting): require complete tier evidence for publication - #77

Open
chrisknvidia wants to merge 8 commits into
mainfrom
fix/christopherk/issue-73-required-tier-evidence
Open

fix(reporting): require complete tier evidence for publication#77
chrisknvidia wants to merge 8 commits into
mainfrom
fix/christopherk/issue-73-required-tier-evidence

Conversation

@chrisknvidia

@chrisknvidia chrisknvidia commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Status

Implementation and review-comment remediation are complete; awaiting maintainer re-review.

This PR is squashed to one signed-off commit: 4f4c44f. All five Mohit/rng1995 review threads have been answered and resolved.

Hosted checks at this SHA: 14 passed and zero are pending. Gitleaks is the only failure; its log reports four synthetic fixtures exclusively from older commits on the separate PR #83 branch because the job fetches and scans every remote branch. A same-version Gitleaks scan restricted to origin/main..HEAD scanned the single PR #77 commit and found no leaks. This PR is not merge-ready until that external check is rerun after the narrow fixture allowlists reach main, and the existing requested-changes reviews are re-evaluated.

Problem

BenchmarkReporter could recommend publication while required tier evidence was absent, unrelated to the selected source, supplied by an arbitrary producer, or represented by spoofable provenance text.

Fix

  • Require completed Tier 1, Tier 2, and Tier 3 publication evidence by default.
  • Bind every certifying result to one exact NFC filesystem-entry name and versioned source-tree digest.
  • Accept Tier 1 and Tier 2 certification only from strict, versioned built-in producer/tier/check-ID markers; unknown results cannot certify or waive policy, while unknown failures still block.
  • Reject reserved provenance placeholders through one shared, pinned Unicode confusable-skeleton check used by reporters and the public benchmark gate.
  • Preserve compatibility- and case-distinct source names across Benchmark, JSON, HTML, and Markdown output.
  • Exclude regular .git pointer files at any depth from both source digests and agent-visible runtime copies while keeping generated report exclusions root-only.
  • Fail closed when required evidence is skipped, malformed, stale, contradictory, incomplete, or bound to another target.

Verification

  • Exact final local tree: 6,083 passed, 19 skipped, 4 deselected.
  • Hosted Python 3.12 and 3.13 suites, native Windows and macOS Tier 2, Tier 3 macOS contract, DCO, CodeQL, dependency review, packaging, RHEL install, and OSS-boundary checks passed.
  • Real CLI success and finding branches generated JSON, HTML, and Markdown reports with canonical producer markers and passed the public benchmark gate.
  • Unicode table regeneration matched the official Unicode 17 source exactly; exhaustive adversarial probes found zero bypasses across 48,326 insertions, 11,756 span replacements, and 11,712 one-for-one substitutions.
  • Fern documentation validation completed with zero errors; 40 docs/packaging tests passed. Redirect validation was skipped because Fern was unauthenticated.
  • Ruff, formatting, diff checks, focused publication/reporting/runtime suites, and independent Review Guru review passed with no remaining P0-P2 findings.
  • Same-version Gitleaks restricted to origin/main..HEAD scanned one commit and found no leaks.

A live credentialed Harbor/provider run was not performed. The real local CLI/report/gate paths and hosted platform contract suites were exercised, but a provider-backed Tier 3 execution remains the integration gap.

Tracking

Fixes #73

@chrisknvidia
chrisknvidia force-pushed the fix/christopherk/issue-73-required-tier-evidence branch from 77f26b8 to c2a546d Compare August 25, 2026 08:18

@mohgupta-ship-it mohgupta-ship-it left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review on behalf of Mohit — REQUEST CHANGES

Critical publication-integrity blocker. I safely reproduced a rendered card that recommends publication for skill-b using a complete Tier 3 payload for skill-b, unrelated Tier 1/Tier 2 results for skill-a, and even a blank default-passing Tier 1 ValidationResult. The public benchmark scanner accepted the resulting PASS card.

The documented split-tier aggregation path needs coherence, not only presence checks. Persist a canonical target/run identity — preferably a content digest plus evaluation identity — for every Tier 1/2/3 result and policy record. Return INCOMPLETE unless all required evidence matches the selected Tier 3 identity, and require recognized Tier 1 execution evidence rather than accepting arbitrary default-passing results. Add mixed-skill, blank-Tier-1, and unrelated-policy regressions.

Comment thread src/skillevaluator/reporting/benchmark.py Outdated
@chrisknvidia
chrisknvidia force-pushed the fix/christopherk/issue-73-required-tier-evidence branch 5 times, most recently from 396ce24 to 9a115b2 Compare August 25, 2026 16:33
@rng1995
rng1995 marked this pull request as ready for review August 25, 2026 18:08

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex review — REQUEST CHANGES

I reproduced four current-head publication-integrity failures. The inline comments include the concrete failure modes and remediation direction. Focused publication/reporting/runtime regressions passed (1,449 passed, 5 skipped), Ruff and diff checks are clean, and all 15 GitHub checks are green.

Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/publication_identity.py Outdated
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
@chrisknvidia
chrisknvidia force-pushed the fix/christopherk/issue-73-required-tier-evidence branch from 9a115b2 to 4f4c44f Compare August 26, 2026 07:24

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head re-review: I verified that all four prior rng1995 findings are genuinely fixed, and those already-resolved threads should remain resolved. The remediated publication/confusable/digest cases and focused suites pass. I found three separate current-head integration defects in the new report identity/output plumbing, so I am requesting changes rather than approving. Verification covered 1,114 publication/reporting/gate tests (2 skipped) plus 423 command/runtime/output tests (4 skipped); Ruff and diff checks are clean. The sole red Gitleaks job comes from unrelated fetched-branch commits, not this PR head.

Comment thread src/skillevaluator/reporting/benchmark.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
@rng1995

rng1995 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

@chrisknvidia Gentle ping when you have a chance: the three current review threads are still waiting for an update or response. Please take a look when you can.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
…ence

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thorough re-review of the current head. All GitHub checks are green and the full local test suite passes, but the inline findings below identify reproducible publication-integrity, report-safety, and CLI/context regressions that need correction before approval. Each publication bypass was reproduced end to end through the reporters and generated benchmark gate.

Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/reporting/base.py Outdated
Comment thread src/skillevaluator/reporting/base.py
Comment thread scripts/ci/check_public_benchmarks.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/cli.py Outdated
Comment thread src/skillevaluator/validators/security.py Outdated
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the complete final diff after all eight requested changes were addressed. Verified publication-evidence reconciliation, Tier 3 runtime identity binding, benchmark-card parser hardening, report-output isolation, scanner completeness semantics, and renderer/path safety. Local full suite: 7,199 passed, 20 skipped, 4 deselected; Ruff, compileall, diff check, and the required public benchmark scan passed. All 17 GitHub checks are green on this exact commit. Approving.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the exact conflict-resolution head after merging current main. The tests/test_commands.py resolution preserves both the malformed-policy regression from main and the PR tier-banner regression, and the full integrated diff has no remaining blocker. Local verification: 7,211 passed, 20 skipped, 4 deselected; Ruff, compileall, diff checks, and the public benchmark scan passed. All 17 GitHub checks are green. Approving.

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.

BenchmarkReporter can recommend publication while Tier 2 is NOT RUN

3 participants