Skip to content

feat: add max_certification_level manifest field#256

Merged
amd-aliem merged 1 commit into
AMDEPYC:mainfrom
amd-aliem:cert-level-cap
Jul 7, 2026
Merged

feat: add max_certification_level manifest field#256
amd-aliem merged 1 commit into
AMDEPYC:mainfrom
amd-aliem:cert-level-cap

Conversation

@amd-aliem

Copy link
Copy Markdown
Contributor

Add optional max_certification_level top-level field to TOML manifests. Tests above the max still run and report results, but do not affect the final certified level.

This change allows tests to be grouped under a certification level without too much coordination.

Currently will cap the 3.0 manifest at 3.0.0-0.

Add optional max_certification_level top-level field to TOML manifests.
Tests above the max still run and report results, but do not affect the
final certified level.

This change allows tests to be grouped under a certification level
without too much coordination.

Currently will cap the 3.0 manifest at 3.0.0-0.

Signed-off-by: Amanda Liem <aliem@amd.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds an optional max_certification_level field to certification TOML manifests and propagates it through the model, CLI execution summary, and report outputs so that certification results are capped (without skipping higher-level test execution).

Changes:

  • Add max_certification_level to CertificationDefinition and validate it against manifest levels.
  • Parse max_certification_level from TOML manifests, preserve it through test filtering, and clamp computed certified level to the cap.
  • Surface max_certification_level in JSON and Markdown outputs, and set a cap for the 3.0 manifest.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
sev_verify/output.py Include max_certification_level in JSON output and display it in Markdown when set.
sev_verify/models.py Add max_certification_level to the certification model with validation logic.
sev_verify/cli.py Parse the new manifest field, preserve it through filtering, and clamp the computed certified level.
sev_verify/cert_tests/c3_0/manifest.toml Define max_certification_level = "3.0.0-0" for the 3.0 test suite manifest.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sev_verify/models.py
@DGonzalezVillal

Copy link
Copy Markdown
Contributor

LGTM
can it be merged?

@amd-aliem amd-aliem merged commit b774e51 into AMDEPYC:main Jul 7, 2026
10 checks passed
@markg-github

Copy link
Copy Markdown
Contributor

Automated review notes (post-merge, for the record)

The feature is well-scoped and the full parse → model → filter → execute → output chain is correctly updated. A few things worth noting:


[MEDIUM] JSON schema change without version bump (output.py:100)

max_certification_level is unconditionally emitted in JSON output (as null when unset), but schema_version stays at "1.0". If any downstream consumer does strict schema validation against "1.0", it will now encounter an unexpected key. Worth either bumping to "1.1" or explicitly documenting that "1.0" is additive-compatible.


[LOW] .index() without a guard (cli.py:563)

if all_levels.index(cap) < all_levels.index(highest):
    highest = cap

Both calls raise ValueError if the value is absent. The trust chain is sound (model validation ensures cap is in all_levels, and highest is only set from values already found there), but a defensive pre-check would make the failure mode explicit rather than relying on invariants being preserved across call sites.


[LOW] Validation silently skips for zero-test manifests (models.py:322)

if self.max_certification_level is not None:
    if self.all_levels and self.max_certification_level not in self.all_levels:
        raise ValueError(...)

The inner if self.all_levels guard means a manifest with max_certification_level set but no tests passes validation silently. Degenerate case (e.g., cap added before tests are written), but could be tightened to raise unconditionally when all_levels is empty and a cap is set.


UX gap: silent clamping

When the cap actually fires (a higher level passes but gets clamped), there's no runtime indication. The cap is announced at execution start, but a user who sees level 3.0.0-1 pass in the per-level table and certified_level: 3.0.0-0 in the summary may be confused. A capped_level field in the JSON (showing the uncapped result) or a brief note in stdout when clamping occurs would make this transparent.


Open questions

  1. Is schema_version: "1.0" intended to be stable/additive, or does every new field warrant a bump?
  2. When 3.0.0-1 tests are eventually added, the existing 3.0.0-0 cap will start actively suppressing their results — is that the intended behavior, or will the cap be removed before those tests land?

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.

4 participants