diff --git a/review/persona.go b/review/persona.go index 8e037a0..0f7f141 100644 --- a/review/persona.go +++ b/review/persona.go @@ -51,7 +51,13 @@ type Severity struct { // LoadPersona loads a persona from a JSON or YAML file path. // Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON. // Files larger than MaxPersonaFileSize are rejected. +// +// Symlinks are supported: os.Stat follows symlinks, so a symlink pointing to +// a regular file will pass the IsRegular() check. Symlinks to non-regular files +// (directories, FIFOs, devices) are still rejected. func LoadPersona(path string) (*Persona, error) { + // os.Stat follows symlinks, so symlinks to regular files are supported. + // The IsRegular() check operates on the target, not the symlink itself. info, err := os.Stat(path) if err != nil { return nil, fmt.Errorf("read persona file %s: %w", path, err) @@ -76,23 +82,15 @@ func LoadPersona(path string) (*Persona, error) { // LoadBuiltinPersona loads a built-in persona by name. // Returns an error if the persona doesn't exist. -// Built-in personas are stored in YAML format. +// Built-in personas are stored in YAML format only (see embed directive). func LoadBuiltinPersona(name string) (*Persona, error) { - // Try YAML first (preferred format) yamlFile := name + ".yaml" data, err := embeddedPersonas.ReadFile("personas/" + yamlFile) - if err == nil { - return parsePersona(data, "builtin:"+yamlFile) - } - - // Fall back to JSON for backwards compatibility - jsonFile := name + ".json" - data, err = embeddedPersonas.ReadFile("personas/" + jsonFile) if err != nil { available := ListBuiltinPersonas() return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", ")) } - return parsePersona(data, "builtin:"+jsonFile) + return parsePersona(data, "builtin:"+yamlFile) } // ListBuiltinPersonas returns the names of all built-in personas in sorted order. @@ -143,7 +141,11 @@ func parsePersona(data []byte, source string) (*Persona, error) { if isYAML { err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth) } else { - err = json.Unmarshal(data, &p) + // Use json.Decoder with DisallowUnknownFields for consistency with + // YAML's KnownFields(true) - both reject unknown fields to catch typos. + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + err = dec.Decode(&p) } if err != nil { return nil, fmt.Errorf("parse persona %s: %w", source, err) diff --git a/review/persona_test.go b/review/persona_test.go index f1f40db..b90eae7 100644 --- a/review/persona_test.go +++ b/review/persona_test.go @@ -693,3 +693,86 @@ severity: }) } } + +func TestJSONUnknownFieldsRejected(t *testing.T) { + tests := []struct { + name string + content string + wantErr string + }{ + { + name: "unknown top-level field", + content: `{ + "name": "test", + "identity": "test identity", + "unknown_field": "should fail" + }`, + wantErr: "unknown_field", + }, + { + name: "typo in field name", + content: `{ + "name": "test", + "identiy": "typo should fail" + }`, + wantErr: "identiy", + }, + { + name: "unknown field in severity", + content: `{ + "name": "test", + "identity": "test", + "severity": { + "major": "ok", + "miner": "typo" + } + }`, + wantErr: "miner", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "test.json") + if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + + _, err := LoadPersona(path) + if err == nil { + t.Fatal("expected error for unknown field, got nil") + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error = %q, want to contain %q", err.Error(), tt.wantErr) + } + }) + } +} + +func TestLoadPersonaSymlink(t *testing.T) { + // Create a regular persona file + dir := t.TempDir() + realFile := filepath.Join(dir, "real.yaml") + content := `name: test +identity: test identity +` + if err := os.WriteFile(realFile, []byte(content), 0644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + + // Create a symlink to it + symlink := filepath.Join(dir, "link.yaml") + if err := os.Symlink(realFile, symlink); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // LoadPersona should work via symlink + p, err := LoadPersona(symlink) + if err != nil { + t.Fatalf("LoadPersona via symlink failed: %v", err) + } + if p.Name != "test" { + t.Errorf("Name = %q, want %q", p.Name, "test") + } +}