Skip to content

fix(schema): make validateConfig validate actual Model structure #191

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 1 commit into
base: main
Choose a base branch
from
+50 −3
Open

fix(schema): make validateConfig validate actual Model structure #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 39 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,42 @@ 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")
}

// 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"},
Comment on lines +619 to +621
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.

This test assumes descriptor.name is required for a valid document, but the current schema/docs treat it as optional. Unless you’re also updating the schema/docs to require name, adjust the tests to validate only the required top-level Model shape (e.g., presence of descriptor, config, and modelfs with non-empty diffIds).

Suggested change
// Test 3: Valid full Model (should pass)
validJSON := `{
"descriptor": {"name": "test-model"},
// Test 3: Valid full Model (should pass). This verifies that a structurally complete
// Model (with descriptor, config, and modelfs with non-empty diffIds) is accepted;
// it does not rely on descriptor.name being required.
validJSON := `{
"descriptor": {},

Copilot uses AI. Check for mistakes.
"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

For better test organization and clearer output on failures, it's a good practice to structure multiple test cases within a single test function using t.Run. This will create sub-tests, each with its own name, making it easier to identify which specific scenario fails.

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.

	t.Run("Incomplete model with only config (should fail)", func(t *testing.T) {
		invalidJSON := `{
			"config": {"paramSize": "8b"}
		}`

		err := schema.ValidatorMediaTypeModelConfig.Validate(strings.NewReader(invalidJSON))
		if err == nil {
			t.Fatalf("expected validation to fail for incomplete model")
		}
	})

	t.Run("Config-only JSON (should fail)", func(t *testing.T) {
		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")
		}
	})

	t.Run("Valid full Model (should pass)", func(t *testing.T) {
		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)
		}
	})
}

14 changes: 11 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,19 @@ 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)
}
Comment on lines 116 to +122
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.

PR description says the current validateConfig behavior allows invalid/incomplete models to pass because it unmarshals into ModelConfig. However, Validator.Validate always runs JSON Schema validation after validateConfig succeeds, and the schema already requires descriptor, config, and modelfs. As written, this change is effectively introducing additional pre-validation rules (e.g., requiring descriptor.name and relying on Go type unmarshalling for diffIds), not just fixing a no-op. Please clarify the intended behavior and ensure it matches the schema/docs.

Copilot uses AI. Check for mistakes.

// 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.

validateConfig now rejects models where descriptor.name is empty, but both the published docs (docs/config.md) and the JSON schema (schema/config-schema.json) describe descriptor.name as OPTIONAL (only minLength: 1 when present). This makes validation stricter than the schema and can reject schema-valid documents. Either remove this check, or update the schema + docs to make descriptor.name required so all validation layers stay consistent.

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 +117 to +129
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.

Because v1.Model.Descriptor and v1.Model.ModelFS are non-pointer structs, json.Unmarshal can’t distinguish between a missing descriptor/modelfs object and an empty one. That leads to potentially misleading errors (e.g., a document missing descriptor entirely will be reported as “missing descriptor.name”). Consider unmarshalling into a lightweight struct with *json.RawMessage/pointer fields so you can accurately detect missing top-level fields and return a clearer error.

Copilot uses AI. Check for mistakes.
}

return nil
Expand Down
Loading