- Clarify depth-aware short-circuit comment to unambiguously describe
the relationship between current depth and previous validation depth
- Add comment to MappingValueNode case explaining intentional depth+2
behavior from parent MappingNode perspective
- Restructure unmarshalYAMLWithDepthLimit doc comment as bullet list
covering all three safety checks (depth, multi-doc, strict fields)
- Replace t.Error with t.Fatal in TestYAMLEmptyFileRejection to remove
redundant nil guard on subsequent err.Error() call
- Add explicit case for *ast.MergeKeyNode in checkYAMLDepth switch to
make it clear this is an intentional leaf (no children to recurse)
rather than relying on the default case. Prevents future library
changes from silently bypassing depth checks.
- Add MaxPersonaFileSize bound check at the top of ParsePersonaBytes.
While callers already check size, the public API should defend itself
(defense in depth) against arbitrarily large inputs that could cause
excessive memory/CPU before AST validation runs.
- Add tests for both behaviors.
Addresses review #2879 findings.
- Add safety note on Strict() decoder not expanding aliases recursively,
since alias resolution uses the pre-validated AST (finding #1)
- Document that ast.Node map keys rely on pointer identity, which holds
because all goccy/go-yaml AST types are pointer receivers (finding #2)
- Clarify AnchorNode comment: effective depth budget is reduced for
anchor+alias pairs, not literally halved (finding #3)
- Improve test depth trace comment for accuracy (finding #4)
- Add HTML comment in CONVENTIONS.md referencing #91 for the two-step
process deviation (finding #5)
MINOR fixes:
- docs/DESIGN-57-yaml-persona.md: fix Error Cases table entry to reflect
custom AST walk (checkYAMLDepth) instead of stale library-level reference
- review/persona.go: add EOF check after JSON decode to reject trailing
garbage after a valid JSON object (prevents silent acceptance of malformed
input like '{"name":"x"}garbage')
- review/persona_test.go: add TestJSONTrailingContentRejected test
NIT fixes:
- review/persona.go: add default case to checkYAMLDepth switch with
explanatory comment about scalar leaf nodes
- review/persona.go: document AnchorNode depth+1 conservative asymmetry
- review/persona.go: simplify redundant if-guard in ListBuiltinPersonas
- Move nodeCount increment after cycle detection to avoid over-counting
cyclic references (sonnet #2)
- Use underscores in test case names used as filenames (sonnet #3)
- Fix function comment: 'prevent silent data loss' → 'prevent confusing
behavior where additional documents are silently ignored' (sonnet #4)
- Mark design doc pseudocode as historical since implementation uses
goccy/go-yaml ast.Node, not gopkg.in/yaml.v3 yaml.Node (sonnet #5)
The global 'seen' set allowed anchored subtrees validated at a shallow
depth to be skipped when later referenced via alias at a greater depth.
This could let effective nesting exceed MaxYAMLDepth, enabling DoS.
Fix: replace the single 'seen' set with two tracking maps:
- validated (node -> min depth): only short-circuits when current depth
<= previously validated depth; re-checks at deeper contexts.
- visiting (node -> bool): per-path recursion stack for true cycle
detection (breaks alias loops without suppressing depth checks).
Add TestYAMLAliasDepthBypass that constructs a document with an
anchored 15-level subtree referenced via alias under 6 levels of
nesting, verifying the combined effective depth (22) is rejected.
Addresses security-review-bot findings on review #2774.
Fixes#87.
PR #58 incorrectly added gopkg.in/yaml.v3 (abandoned library) instead of
github.com/goccy/go-yaml as required by issue #57.
Changes:
- Replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml v1.19.2
- Update review/persona.go to use goccy/go-yaml API:
- parser.ParseBytes for AST-based depth/node count checking
- yaml.Strict() decoder option instead of KnownFields(true)
- ast.Node types instead of yaml.Node for tree walking
- Update review/persona_test.go to use ast types for cycle tests
- Remove gopkg.in/yaml.v3 from go.mod and go.sum
All existing YAML tests pass with the new library.
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
Address security review findings:
MAJOR: Add cycle detection to checkYAMLDepth using a visited set
(seen map[*yaml.Node]struct{}) to prevent infinite recursion from
crafted YAML with self-referential aliases.
MINOR fixes:
- Add MaxYAMLNodes (1000) limit as defense-in-depth against
wide-but-shallow structures that bypass depth limits
- Increment depth when following alias targets (was incorrectly
passing same depth, allowing alias chains to bypass depth limit)
- Reject multi-document YAML files instead of silently ignoring
additional documents (prevents confusing silent data loss)
Tests added:
- TestYAMLAliasCycleDetection: Direct test of cycle detection logic
- TestYAMLMultiDocumentRejection: Verifies multi-doc files rejected
- TestYAMLNodeCountLimit: Verifies wide structures are rejected
- TestCheckYAMLDepthCycleDetectionDirect: Unit test with artificial cycle
Addresses PR #58 MINOR finding: YAML decoder now rejects unknown fields.
- Enable KnownFields(true) on YAML decoder to catch typos like
'focuss' or 'identiy' in persona files
- Since yaml.Node.Decode() doesn't support KnownFields, we now
do a two-pass decode: first pass checks depth limits, second
pass decodes with strict field checking
- Add tests for unknown field rejection at top-level and nested levels
MAJOR fixes:
- Remove false security claim about gopkg.in/yaml.v3 having built-in depth protection
- Add explicit YAML depth limiting via yaml.Node API (MaxYAMLDepth=20)
- Add file size limit for persona files (MaxPersonaFileSize=64KB)
- Add test for deeply nested YAML rejection
MINOR fixes:
- Add sort.Strings to ListBuiltinPersonas for deterministic ordering
- Update design doc to reflect actual library used (gopkg.in/yaml.v3)
- Update README: 'Zero dependencies' → 'Minimal dependencies'
- Add test for file size limit
- Add test for sorted persona list
- Add gopkg.in/yaml.v3 dependency (approved in CONVENTIONS.md)
- Update parsePersona to detect format by file extension
- Support both .yaml and .yml extensions (case-insensitive)
- Convert built-in personas to YAML format
- Add comprehensive tests for YAML parsing
- Update README with YAML examples and documentation
YAML provides cleaner multi-line strings via literal block scalars
and supports comments, making persona definitions more readable.
JSON remains supported for backwards compatibility.
Closes#57
MAJOR fixes:
- Remove external YAML dependency (github.com/goccy/go-yaml)
Per project convention: Go standard library only, zero dependencies.
Convert all persona files from YAML to JSON format.
- Fix TestValidateWorkspacePath error expectation
Go 1.21+ filepath.Join normalizes absolute paths differently.
MINOR fixes:
- Remove custom contains helper in persona_test.go (use strings.Contains)
- Add Unicode-safe CapitalizeFirst function for header titles
- ListBuiltinPersonas returns empty slice instead of nil on error
- Fix test comment about filepath.Join behavior
Documentation:
- Update README to reflect JSON-only persona format
- Update design doc with note about JSON decision
- Fix action.yml description for persona-file input
Add persona system for specialized review roles. Each persona defines:
- A specific review focus (security, architecture, documentation)
- Custom system prompt additions
- Personality/tone adjustments
Built-in personas: security, architect, docs
Custom personas: load from JSON via persona-file flag
Includes workspace validation to prevent path traversal attacks.
Closes#51
claw
Rodin
Aaron Weiker
Rodin