Skip to content

fix(schema): validate OCI digest format for modelfs.diffIds #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rishi-jat wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(schema): validate OCI digest format for modelfs.diffIds #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f5542b
fix(schema): make validateConfig validate actual Model structure
rishi-jat Mar 22, 2026
113d8de
fix(schema): validate OCI digest format for modelfs.diffIds
rishi-jat Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions schema/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,3 +589,139 @@ func TestConfig(t *testing.T) {
}
}
}

func TestValidateConfigParsesModelNotModelConfig(t *testing.T) {
// This test verifies that validateConfig correctly parses the full Model structure,
// not just ModelConfig. Previously, validateConfig unmarshaled into ModelConfig,
// which always succeeded because all fields are optional.

// Test 1: Incomplete model with only config (should fail)
invalidJSON := `{
"config": {"paramSize": "8b"}
}`

err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidJSON))
if err == nil {
t.Fatalf("expected validation to fail for incomplete model")
}
Comment on lines +593 to +606
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

TestValidateConfigParsesModelNotModelConfig is described as verifying the switch from unmarshaling ModelConfig to Model, but the assertions only check err != nil for invalid inputs. This test would still pass even if validateConfig unmarshaled into ModelConfig, because schema validation already rejects those payloads. Consider asserting on the specific failure reason (e.g., the pre-validation error) so the test actually protects the intended behavior.

Copilot uses AI. Check for mistakes.

// Test 2: Config-only JSON (should fail)
configOnlyJSON := `{
"paramSize": "8b",
"architecture": "transformer"
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(configOnlyJSON))
if err == nil {
t.Fatalf("expected failure for config-only JSON without descriptor/modelfs, but got nil")
}

// Test 3: Valid full Model (should pass)
validJSON := `{
"descriptor": {"name": "test-model"},
"config": {"paramSize": "8b"},
"modelfs": {"type": "layers", "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"]}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(validJSON))
if err != nil {
t.Fatalf("expected valid Model to pass, but got error: %v", err)
}
}
Comment on lines +593 to +630
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

This test function contains several related test cases. To improve readability and maintainability, consider refactoring it into a single table-driven test. This is a common Go idiom that makes it easier to add, remove, or modify test cases in the future.

func TestValidateConfigParsesModelNotModelConfig(t *testing.T) {
	// This test verifies that validateConfig correctly parses the full Model structure,
	// not just ModelConfig. Previously, validateConfig unmarshaled into ModelConfig,
	// which always succeeded because all fields are optional.
	testCases := []struct {
		name       string
		jsonInput  string
		shouldFail bool
	}{
		{
			name:       "Incomplete model with only config",
			jsonInput:  `{"config": {"paramSize": "8b"}}`,
			shouldFail: true,
		},
		{
			name:       "Config-only JSON",
			jsonInput:  `{"paramSize": "8b", "architecture": "transformer"}`,
			shouldFail: true,
		},
		{
			name:       "Valid full Model",
			jsonInput:  `{"descriptor": {"name": "test-model"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"]}}`,
			shouldFail: false,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(tc.jsonInput))
			if tc.shouldFail {
				if err == nil {
					t.Fatalf("expected validation to fail for '%s', but it passed", tc.name)
				}
			} else {
				if err != nil {
					t.Fatalf("expected validation to pass for '%s', but it failed with: %v", tc.name, err)
				}
			}
		})
	}
}


func TestValidateDigestFormat(t *testing.T) {
// Test that validateConfig rejects invalid OCI digest formats

// Test 1: Invalid digest format (no algorithm:hex format)
invalidDigestJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": ["invalid-digest"]
}
}`

err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidDigestJSON))
if err == nil {
t.Fatalf("expected failure for invalid digest format")
}

// Test 2: Invalid hex in digest
invalidHexJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": ["sha256:xyz"]
}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidHexJSON))
if err == nil {
t.Fatalf("expected failure for invalid hex in digest")
}

// Test 3: Empty hash
emptyHashJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": ["sha256:"]
}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(emptyHashJSON))
if err == nil {
t.Fatalf("expected failure for empty hash in digest")
}

// Test 4: Multiple invalid digests
multipleInvalidJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": ["invalid", "also-invalid"]
}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(multipleInvalidJSON))
if err == nil {
t.Fatalf("expected failure for multiple invalid digests")
}

// Test 5: Valid digest (should pass)
validJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"]
}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(validJSON))
if err != nil {
t.Fatalf("expected valid digest to pass, got: %v", err)
}

// Test 6: Multiple valid digests (should pass)
multipleValidJSON := `{
"descriptor": {"name": "test"},
"config": {"paramSize": "8b"},
"modelfs": {
"type": "layers",
"diffIds": [
"sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef",
"sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"
]
}
}`

err = schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(multipleValidJSON))
if err != nil {
t.Fatalf("expected multiple valid digests to pass, got: %v", err)
}
}
Comment on lines +632 to +727
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

This test function contains several similar test cases. To improve readability and maintainability, consider refactoring it into a single table-driven test. This is a common Go idiom that makes it easier to add, remove, or modify test cases in the future.

func TestValidateDigestFormat(t *testing.T) {
	// Test that validateConfig rejects invalid OCI digest formats
	testCases := []struct {
		name       string
		jsonInput  string
		shouldFail bool
	}{
		{
			name:       "Invalid digest format (no algorithm:hex format)",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["invalid-digest"]}}`,
			shouldFail: true,
		},
		{
			name:       "Invalid hex in digest",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["sha256:xyz"]}}`,
			shouldFail: true,
		},
		{
			name:       "Empty hash",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["sha256:"]}}`,
			shouldFail: true,
		},
		{
			name:       "Multiple invalid digests",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["invalid", "also-invalid"]}}`,
			shouldFail: true,
		},
		{
			name:       "Valid digest",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef"]}}`,
			shouldFail: false,
		},
		{
			name:       "Multiple valid digests",
			jsonInput:  `{"descriptor": {"name": "test"}, "config": {"paramSize": "8b"}, "modelfs": {"type": "layers", "diffIds": ["sha256:1234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef", "sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"]}}`,
			shouldFail: false,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(tc.jsonInput))
			if tc.shouldFail {
				if err == nil {
					t.Fatalf("expected failure for '%s'", tc.name)
				}
			} else {
				if err != nil {
					t.Fatalf("expected valid case '%s' to pass, got: %v", tc.name, err)
				}
			}
		})
	}
}

21 changes: 18 additions & 3 deletions schema/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,26 @@ var validateByMediaType = map[Validator]validateFunc{
}

func validateConfig(buf []byte) error {
mc := v1.ModelConfig{}
var model v1.Model

err := json.Unmarshal(buf, &mc)
err := json.Unmarshal(buf, &model)
if err != nil {
return fmt.Errorf("config format mismatch: %w", err)
return fmt.Errorf("invalid model structure: %w", err)
}

// Minimal structural validation for required fields
if model.Descriptor.Name == "" {
return fmt.Errorf("missing descriptor.name")
}
Comment on lines +125 to +127
Copy link

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

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

The new model.Descriptor.Name == "" check makes descriptor.name effectively required, but the JSON schema allows an empty descriptor object (no required list for name, only minLength if present). This introduces a stricter validation rule than the schema/spec and can reject inputs that would otherwise pass schema validation. Either drop this check, or update the schema/spec to require descriptor.name and adjust tests accordingly.

Suggested change
if model.Descriptor.Name == "" {
return fmt.Errorf("missing descriptor.name")
}

Copilot uses AI. Check for mistakes.
if len(model.ModelFS.DiffIDs) == 0 {
return fmt.Errorf("missing modelfs.diffIds")
}

Comment on lines +124 to +131
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Mar 22, 2026

Choose a reason for hiding this comment

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

medium

These manual checks for descriptor.name and modelfs.diffIds are redundant. The JSON schema (config-schema.json) already enforces that descriptor.name is non-empty (minLength: 1) and modelfs.diffIds is not empty (minItems: 1). Since validateSchema is called after this function, these checks are duplicated.

To improve maintainability and avoid logic duplication, it's best to remove these manual checks and rely on the schema validation. This function should only contain validation logic that cannot be expressed in the JSON schema, like the digest format check.

// Validate digest format for each diffId
for _, d := range model.ModelFS.DiffIDs {
if err := d.Validate(); err != nil {
return fmt.Errorf("invalid diffId %q: %w", d, err)
}
}

return nil
Expand Down
Loading