fix: address remaining PR #58 review findings
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 9m31s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 9m54s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m40s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 11m27s

1. Remove dead JSON fallback in LoadBuiltinPersona
   - The embed directive only includes *.yaml files
   - JSON fallback code could never succeed
   - Simplified function to only try YAML

2. JSON parsing now rejects unknown fields
   - Switched from json.Unmarshal to json.Decoder
   - DisallowUnknownFields() matches YAML's KnownFields(true)
   - Added test coverage for JSON unknown field rejection

3. Documented symlink support in LoadPersona
   - os.Stat follows symlinks, so symlinks to regular files work
   - Added doc comment explaining the behavior
   - Added test for symlink support
This commit is contained in:
Rodin
2026-05-10 17:53:42 -07:00
parent 26f326cf51
commit 10cd6203d4
2 changed files with 96 additions and 11 deletions
+13 -11
View File
@@ -51,7 +51,13 @@ type Severity struct {
// LoadPersona loads a persona from a JSON or YAML file path. // 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. // Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON.
// Files larger than MaxPersonaFileSize are rejected. // 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) { 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) info, err := os.Stat(path)
if err != nil { if err != nil {
return nil, fmt.Errorf("read persona file %s: %w", path, err) 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. // LoadBuiltinPersona loads a built-in persona by name.
// Returns an error if the persona doesn't exist. // 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) { func LoadBuiltinPersona(name string) (*Persona, error) {
// Try YAML first (preferred format)
yamlFile := name + ".yaml" yamlFile := name + ".yaml"
data, err := embeddedPersonas.ReadFile("personas/" + yamlFile) 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 { if err != nil {
available := ListBuiltinPersonas() available := ListBuiltinPersonas()
return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", ")) 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. // 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 { if isYAML {
err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth) err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
} else { } 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 { if err != nil {
return nil, fmt.Errorf("parse persona %s: %w", source, err) return nil, fmt.Errorf("parse persona %s: %w", source, err)
+83
View File
@@ -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")
}
}