[MINOR] isNotFoundError uses string-based heuristics ("not found" case-insensitive substring match). This could produce false positives — e.g., an error message like "operation not found in registry" from a completely different layer would be treated as a 404. The comment acknowledges this is a heuristic, which is acceptable given the import cycle constraint, but consider wrapping gitea errors with a typed sentinel or checking only for the more specific "HTTP 404" prefix to reduce false positives.
[NIT] The giteaClientAdapter struct and its constructor/methods could live in a separate file (e.g., cmd/review-bot/adapter.go) to keep main.go focused, but this is a very minor organizational concern.
[MINOR] When LoadRepoPersonas returns an error (non-404), the code logs a warning and continues with repoPersonas being nil (its zero value). The subsequent if p, ok := repoPersonas[*personaName] safely handles nil maps (map indexing on nil returns zero value), so this works correctly, but it's subtle. A reader might wonder if repoPersonas could cause a panic. Consider initializing repoPersonas to an empty map before the error check, or adding a brief comment that nil map indexing is safe in Go.
[NIT] Minor style: extra blank line in the TestParsePersonaBytes test struct definition between wantName and wantErr fields. This is inconsistent with the other test structs in the file and would be flagged by gofmt.
[NIT] The GiteaClient interface name is somewhat generic for a package named review. Since this interface is exported and lives in review, callers see it as review.GiteaClient. A name like RepoFileClient or ContentClient would be more descriptive of what it actually does (list/get file contents) rather than being named after a specific vendor.
[NIT] isNotFoundError uses a string-based heuristic (strings.Contains(errStr, "not found")) that could produce false positives for errors like "user not found" from application logic. The HTTP 404 check is precise; the bare not found check is looser. This is acknowledged in the comment but worth considering whether to tighten to only the HTTP 404 pattern.
[NIT] Minor formatting inconsistency: the TestParsePersonaBytes table struct has inconsistent field alignment (extra blank line between source and wantName fields in some entries). This is cosmetic and gofmt should handle it, but it's visible in the raw source.
[MINOR] When LoadRepoPersonas returns an error, repoPersonas will be nil (not an empty map), but the subsequent repoPersonas[*personaName] map index on line ~187 is safe in Go (indexing a nil map returns the zero value, false). However, the err variable from LoadRepoPersonas is shadowed by the later persona, err = review.LoadBuiltinPersona(...) assignment at line ~193, which is correct since the outer err is already declared. The logic is sound but slightly subtle — consider using a blank identifier or a named variable repoErr to make the intent clearer.
[MINOR] The multi-document check dec.Decode(&extra) == nil consumes IO from the first-pass decoder, but the strict second-pass re-decodes from bytes.NewReader(data), so correctness is fine. However, the document count check is somewhat brittle: it silently ignores any decode error on the second document (including genuine IO errors) by treating any non-nil error as 'only one document'. In practice this works since the reader is in-memory, but the intent would be clearer with if err := dec.Decode(&extra); !errors.Is(err, io.EOF) && err == nil { return fmt.Errorf(...) } or by explicitly checking err == nil.
[NIT] Both TestYAMLAliasCycleDetection and TestCheckYAMLDepthCycleDetectionDirect test nearly identical scenarios (artificial AliasNode cycle). They could be merged into one test with subtly different configurations to reduce duplication.
[NIT] The cycle detection comment says 'Already validated this subtree, skip to avoid infinite recursion' but returning nil on a revisited node means that node's depth contribution is not re-checked at the new depth. This is intentional (and correct for cycle-breaking), but the comment could be clearer that depth checking for shared/aliased subtrees is only done once from the first encounter.
[NIT] The design doc's Error Cases table says 'Deeply nested YAML
[MINOR] The ListBuiltinPersonas deduplication map (seen[personaName] = true) is correct but slightly redundant: the condition if !seen[personaName] is always true on the first encounter (the only time the body runs), so the inner check adds noise. A simpler pattern would be if !seen[personaName] { seen[personaName] = true; names = append(names, name) }, building the slice directly instead of a two-pass map → slice conversion. Not a bug, just unnecessary complexity.
[NIT] The design doc's error table says 'Deeply nested YAML
[NIT] The test TestLoadPersonaFromJSONFile has a blank line added inside the JSON content string ("focus": ["testing", "validation"],\n\n"ignore":...). While valid JSON, the blank line is unnecessary and looks like an accidental edit.
[MINOR] The multi-document rejection check (dec.Decode(&extra)) re-uses the same decoder after the first dec.Decode(&node) call. If the first document exhausted the decoder but there's trailing non-document content (e.g., a trailing ... end-of-document marker), the behavior depends on go-yaml internals. This is likely fine in practice, but the comment could clarify that this relies on go-yaml's decoder advancing past the first document.