[samuv]

Follow-on to #247 and #248. Small, and mostly correcting documentation that those two left inconsistent.

Why

#247 stripped attestation from the skill provenance contract because the verifier could silently skip it — an expected attestation was only compared when the artifact also carried one, so the constraint was dropped exactly when it mattered:

if p.Attestation != nil && r.Statement != nil && p.Attestation.Predicate != nil && r.Statement.Predicate != nil {

#248 closed that gap. compareAttestation now returns false when the provenance asks for an attestation and r.Statement == nil.

That removed the reason for the restriction but not the restriction — and it left two comments on main asserting the opposite of what the code now does:

• skill.schema.json, $defs/provenance: "the verifier only compares an expected attestation when the artifact also carries one, so a declared attestation constraint would be silently skipped"
• skills_types.go: "Only the identity fields are honored… Refusing it loudly keeps the contract fail-closed until that gap is fixed"

The gap is fixed. This PR re-adds the key and corrects both.

Deliberately stricter than the server schema

The skill verified_attestation definition types predicate as an object, which publisher-provided.schema.json does not. This isn't gratuitous — the verifier normalizes an expected predicate through structpb.NewStruct, which requires map[string]any. Checked against the merged code:

string(str)          -> err=expected predicate is string, want an object
int(42)              -> err=expected predicate is int, want an object
[]interface{}([a])   -> err=expected predicate is []interface {}, want an object
map[string]any{k:v}  -> ok

So a non-object predicate mismatches every artifact. Left untyped, the schema would happily accept a constraint that can never be satisfied, and the failure would surface at install time as an unexplained verification failure with nothing pointing at the catalog entry. Typing it turns that into a validation error at authoring time.

Worth considering the same tightening for the server schema separately — I haven't touched it here, since it's a live contract and out of this PR's scope.

additionalProperties: false stays on both definitions. It was added for typo protection, not for the attestation restriction, and it now also catches a misspelled predicatetype.

Semantics (matching #248 exactly)

Expected	Behavior
attestation absent	unconstrained
attestation: {}	artifact must be attested; no constraint on content
predicate_type set	must equal the statement's predicate type
predicate set	must deep-equal the statement predicate (both normalized through structpb)

Tests

TestSkill_ValidateWithProvenance — attestation cases flipped from expectError back to success, plus empty attestation constrains only presence, predicate type without predicate, and rejection of scalar/array predicates.

TestValidateSkillBytes_Provenance — valid full and empty attestation constraints; rejection of a non-object attestation, a non-object predicate, and a misspelled predicatetype.

Verification

• task — green, 0 lint issues.
• task license-check — passes.
• registry/types coverage: 81.1%.
• Existing TestSkill_Validate untouched and passing.

Note on release sequencing

v0.0.41 predates #247 and #248, so neither is released yet. Landing this before the next tag means the skill provenance contract ships complete in one version, rather than shipping a restriction that's immediately relaxed in the next — worth doing before cutting the release that unblocks stacklok/toolhive#6310's go.mod bump.

🤖 Generated with Claude Code