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
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:
+13
-11
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user