validateurl.go is VCS-generic but imported gitea.IsBlockedIP, creating an
unexpected generic→Gitea-specific dependency. Extract IsBlockedIP and its
CIDR list to internal/netutil/ipcheck.go (a neutral shared package).
- gitea/ipcheck.go becomes a thin forwarding wrapper (preserves API compat
for callers within the gitea package)
- gitea/ipcheck_test.go replaced with a forwarding smoke test; full coverage
moves to internal/netutil/ipcheck_test.go
- validateurl.go now imports internal/netutil directly
The field was named NewPosition with a misleading comment 'Gitea: absolute
line; GitHub: diff hunk position'. In reality both adapters use it as an
absolute new-file line number (Gitea maps it to new_position, GitHub maps it
to Line+Side:RIGHT). Rename to NewLine to match actual semantics and update
comments to explain per-adapter mapping.
- CLI example used $GITEA_TOKEN which is not an actual env var; rename to
$REVIEWER_TOKEN (the correct env var the binary reads)
- Env var table referenced GITEA_REPO without noting GitHub support; add
a note and include VCS_TYPE row so users know they can override detection
The binary detects VCS type from VCS_TYPE env var, but action.yml did not
pass it to the Run review step. This caused the binary to fall back to a
URL heuristic (github.com substring), which misclassifies GitHub Enterprise
Server hosts whose URL does not contain 'github'.
The 'Determine version' step already outputs vcs_type — wire it through to
the Run review env block so explicit VCS_TYPE always takes precedence.
Address security-review-bot REQUEST_CHANGES findings on PR #142:
MAJOR (Finding #1): Docmap file path was read directly without validating it
is within --repo-root or checking for symlinks. A malicious PR could create
.review-bot/doc-map.yml as a symlink to /dev/zero (resource exhaustion) or an
arbitrary host file (information disclosure).
Fix: Add validateDocmapPath() called before ParseDocMapConfig(). It:
- Resolves --repo-root first (filepath.Abs + EvalSymlinks), moved up before
docmap parsing so both checks share the same resolved root
- Uses os.Lstat to detect symlinks and rejects them outright
- Confirms the docmap path is within resolvedRoot via filepath.Rel
- Checks file size against maxDocmapBytes (10 MB) before reading
MINOR (Finding #2): No upper bound on docmap YAML size.
Fix: os.Lstat size check enforces maxDocmapBytes cap before os.ReadFile.
Tests:
- TestValidateDocmapPath_Symlink: docmap is a symlink → exit 2
- TestValidateDocmapPath_OutsideRepoRoot: docmap outside repo-root → exit 2
- TestValidateDocmapPath_SizeLimit: docmap exceeds 10 MB cap → exit 2
- Updated all existing tests to use makeDocmapInDir() so the docmap
lives inside the repo-root, satisfying the new confinement check
Finding #1 [MAJOR]: replace os.Stat with os.Lstat in checkStaleDocs to
prevent symlink traversal. Symlinks under repoRoot could probe arbitrary
host file existence; Lstat never follows them. Symlinked docs are now
treated as stale.
Finding #2 [MINOR]: resolve --repo-root with filepath.Abs +
filepath.EvalSymlinks before passing to checkStaleDocs, so a symlinked
repo-root cannot bypass the filepath.Rel escape guard.
Finding #3 [NIT]: reject backslashes in ValidateDocPath to prevent
Windows platform edge cases where a path separator may be normalised
differently by the host OS or VCS backend.
Tests added:
- TestCheckStaleDocs_SymlinkOutside: symlink inside repo → outside
- TestCheckStaleDocs_SymlinkInsideRepo: intra-repo symlink also rejected
- TestRunValidateDocmap_SymlinkRepoRoot: symlinked --repo-root resolves OK
- TestValidateDocPath_Backslash: backslash paths rejected
- Backslash cases added to TestValidateDocPath invalid slice
All go test ./... pass, go vet ./... clean.
Export review.ValidateDocPath and use it in checkStaleDocs before
calling os.Stat. Add filepath.Clean + filepath.Rel confinement check
as defense-in-depth to ensure doc paths from PR-controlled YAML
cannot probe filesystem locations outside repoRoot.
Also add tests covering: ../../etc/passwd, /etc/passwd, ../outside,
a valid present path, and a valid missing path.
Addresses security finding from security-review-bot on PR #142.
TestRunValidateDocmap_Clean was reading real os.Stdin (fragile in CI).
Switch to stdinValidateDocmap with a covered file and empty-stdin test
already covered by TestRunValidateDocmap_EmptyStdin.
Adds 'review-bot validate-docmap' for CI hard-fail on docmap coverage gaps.
Usage:
git diff --name-only origin/main HEAD | \
review-bot validate-docmap --docmap .review-bot/doc-map.yml --repo-root .
Flags:
--docmap (required) path to doc-map YAML file
--repo-root (optional, default '.') root for resolving docs: paths
Two checks, both always run:
1. Coverage: every stdin file must match at least one paths: glob.
2. Stale docs: every docs: entry must exist on disk under --repo-root.
Exit codes: 0=clean, 1=failures found, 2=usage/parse error.
Tests cover: clean pass, uncovered file, stale doc, both failures,
empty stdin, blank-line stdin, and duplicate docs: deduplication.
Adds FileCoveredByDocMap(cfg *DocMapConfig, file string) bool — a thin wrapper
over the existing unexported mappingMatches that lets cmd/ check per-file docmap
coverage without duplicating glob logic.
Also adds unit tests covering matched globs, non-matching paths, empty file,
and empty config.
- New --doc-map flag (DOC_MAP_FILE env var): path to YAML config mapping
source path globs to governing design docs
- New --doc-map-max-bytes flag (DOC_MAP_MAX_BYTES env var): cap on total
injected doc content, default 100KB
- review/docmap.go: DocMapConfig parsing, glob matching with ** support,
doc loading via VCS with directory expansion and size guard
- budget.Sections: new DesignDocs field, trimmed after conventions
- budget.buildResult: injects DesignDocs under ## Design Documents heading
- action.yml: doc-map and doc-map-max-bytes inputs wired to env vars
- CHANGELOG.md: created with unreleased entry
- Tests: ParseDocMapConfig, MatchDocs, globMatch, LoadMatchingDocs
gitea: Add 4 tests for GetTimelineReviewCommentIDForReview (was 0% coverage):
- Success: find review in timeline by user login + body prefix match
- ReviewFetchError: 404 on review API
- EmptyBody: review with empty body returns error
- NotFoundInTimeline: body matches but user login doesn't
github: Add 3 tests for GetAllFilesInPath (was 0% coverage):
- DirectoryWithFiles: lists directory, fetches base64-encoded file content
- 404FallsBackToFile: 404 on dir path returns error when file also 404s
- DirectoryWithSubdir: recursive directory traversal
Coverage changes:
- gitea: 80.0% → 85.2%
- github: 79.9% → 86.3%
Python's ipaddress module does NOT classify 100.64.0.0/10 (RFC6598
carrier-grade NAT) as private/loopback/link_local/multicast/reserved.
This means a SERVER_URL resolving to a CGN address would bypass the
Python SSRF check and reach curl with ACTION_TOKEN.
Add an explicit network membership check for 100.64.0.0/10 to both
Python validation blocks in action.yml:
- _ssrf_check.py (VCS URL pre-flight check)
- _ssrf_check_install.py (binary download URL check)
The Go-level IsBlockedIP already covers this range correctly (ipcheck.go);
this fix closes the gap in the action.yml Python layer.
Also update comments to mention RFC6598 explicitly.
- Add --vcs-url flag as primary (reads VCS_URL env var)
- Keep --gitea-url and GITEA_URL as deprecated fallbacks with warnings
- Update action.yml: rename gitea-url input to vcs-url, pass VCS_URL to binary
- Update ci.yml: use VCS_URL env var in Run review step
- Update integration tests: INTEGRATION_GITEA_URL -> INTEGRATION_VCS_URL
- Update README: --vcs-url / VCS_URL with fallback note in env var table
Backward compat: existing GITEA_URL users get a deprecation warning and
continue to work unchanged until they migrate to VCS_URL.
Add defensive check for empty Name and Path fields when unmarshaling
a single ContentEntry in the fallback path. While Gitea API won't
return empty objects for valid file paths, this guard:
- Explicitly documents the invariant we expect
- Catches potential API behavior changes early
- Costs nothing at runtime
Addresses [MINOR] from sonnet-review-bot on PR #74.
When ListContents is called with a path that points to a file (not a
directory), Gitea returns a single JSON object instead of an array.
Previously this caused json.Unmarshal to fail with:
json: cannot unmarshal object into Go value of type []gitea.ContentEntry
Now ListContents tries array unmarshal first, and falls back to single
object unmarshal, wrapping it in a slice. This allows patterns-files
config to specify individual files like 'README.md' without triggering
a parse error.
Also updates TestGetAllFilesInPath_File to reflect actual Gitea behavior
(single object response, not 404).
Fixes#73
Gitea API rejects "." with HTTP 500 (malformed path component).
When patterns-files is set to ".", normalize it to empty string
before making the API call.
Fixes#70
Move lastErr assignment outside the retry condition so that both
network errors and HTTP 5xx paths return lastErr consistently.
Previously, on the final retry attempt, a network error would return
the raw err variable instead of lastErr. While they held the same
value in practice, the inconsistency was confusing when reading the
code.
Now both paths:
- Network errors: assign lastErr before checking retry, return lastErr
- HTTP 5xx: assign lastErr before checking retry, return lastErr
Addresses review finding #3 (MINOR) from sonnet review on PR #69.
1. Fix non-deterministic test TestDoGet_RetriesOnTemporaryNetError:
- Replace timing-dependent listener approach with mockTransport
- mockTransport allows controlled injection of net.OpError failures
- Test now makes deterministic assertions: exactly 3 attempts (2 fail + 1 success)
- Added SetHTTPClient() method for test transport injection
2. Sanitize error content in retry warning logs:
- Added sanitizeErrorForLog() helper that omits response body content
- For APIError: logs only 'HTTP <status>' instead of full body
- For other errors: preserves error type information
- Addresses security concern about logging server error content at WARN level
- Full error with body still returned to caller for proper error handling
Both changes have corresponding test coverage.
Address review feedback on isTemporaryNetError being too broad:
1. RetryBackoff field: Added doc comment clarifying it must be
configured before the first request (addresses concurrency concern).
2. isTemporaryNetError: Now inspects the underlying syscall error
instead of treating all net.OpError as retriable. Only retries on:
- ECONNREFUSED (connection refused)
- ECONNRESET (connection reset)
- ENETUNREACH (network unreachable)
- EHOSTUNREACH (host unreachable)
- ETIMEDOUT (connection timed out)
Permanent errors like EACCES, EPERM are no longer retried.
3. DNS errors: Changed from Temporary() to IsTimeout, since
"no such host" is permanent and shouldn't be retried.
4. Empty backoff slice: Added comment explaining that retry without
delay is intentional when caller explicitly configures it.
Addresses MINOR findings from sonnet-review-bot and gpt-review-bot.
Address review feedback:
1. Make backoff delays injectable via Client.RetryBackoff field
- Defaults to {1s, 2s} when nil for production
- Tests can set shorter values for fast execution
- Fixes slow unit tests that previously waited 3+ seconds
2. Add retry on temporary network errors (net.OpError, net.DNSError)
- Connection refused, network unreachable, DNS failures now retry
- Non-temporary network errors still fail immediately
- Context cancellation still respected during backoff
Added isTemporaryNetError helper and TestIsTemporaryNetError test.
Updated existing retry tests to use configurable short backoffs.
- Remove dead backoff[0] element; array now only contains retry delays
- Fix time.After timer leak by using time.NewTimer with timer.Stop()
- Add io.LimitReader (64KB) for error body reads to bound memory allocation
Addresses feedback from sonnet-review-bot, security-review-bot, and gpt-review-bot.
The read:user scope is needed for the bot to self-request as a
reviewer on PRs. Without it, the bot still functions but cannot
add itself to the reviewer list.
Closes#66
When patterns-repo is configured, now logs at Info level:
- File paths loaded from each repo
- Count of files per repo
At Debug level logs skipped files (non-markdown/txt/yaml).
Warns if no pattern files were loaded from a repo (likely
misconfigured patterns-files path).
Closes#64
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
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
Add native SAP AI Core provider that handles OAuth token management and
deployment discovery automatically. This eliminates the need for the
external LLM proxy when running in SAP environments.
Changes:
- Add AICoreClient with OAuth token caching and deployment URL discovery
- Support both Anthropic and OpenAI models via AI Core deployments
- Update CI to use native AI Core provider
- Update action inputs to accept AI Core credentials
- Update README with AI Core configuration examples
Model names must match AI Core deployment names (e.g. anthropic--claude-4.6-sonnet, gpt-5).
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
When a PR is pushed after being marked self-reviewed, the label is now
stale and should be removed. This matches the gargoyle CI behavior.
On synchronize:
- Remove self-reviewed label if present
- Reassign PR back to the author
- Restore sonnet reviewer with correct model name (anthropic--claude-4.6-sonnet)
- Remove gpt-4.1, gpt-4.1-mini, gpt-5-mini (not deployed on SAP AI Core)
- Keep gpt-5 and security reviewers
The previous model names (claude-sonnet-4-6, etc.) were incorrect —
SAP AI Core uses 'anthropic--claude-4.6-sonnet' format.
Models claude-sonnet-4-6, gpt-4.1, gpt-4.1-mini, and gpt-5-mini are not
deployed on the LLM proxy, causing 502 errors. Keep only gpt-5 which
is the only available model.
- Fix token_secret for gpt41/gpt5-mini/gpt41-mini: use GPT_REVIEW_TOKEN
instead of SONNET_REVIEW_TOKEN (wrong reviewer identity)
- Move LLM base URL back to secrets.LLM_BASE_URL (prevents exfiltration
via PR-controlled matrix values)
- Remove hardcoded internal IP from workflow file; only provider path
suffix (/anthropic/v1, /openai/v1) remains in matrix
Addresses: security-review-bot REQUEST_CHANGES (major: exfiltration risk,
minor: HTTP/hardcoded IP) and sonnet-review-bot REQUEST_CHANGES (major:
wrong token_secret on gpt entries).
The matrix was wrong: "sonnet" was running GPT-5 and "gpt" was running
GPT-4.1. Now:
- sonnet → Claude Sonnet 4.6 via HAI Anthropic endpoint
- gpt → GPT-5 via HAI OpenAI endpoint
- security → GPT-5 via HAI OpenAI endpoint
Each matrix entry specifies its own provider and base_url.
Previously findOwnReview returned only the single most-recent matching
review, so on PRs with multiple force-pushes only the latest old review
got superseded. The rest accumulated as unsuperseded stale reviews.
Changes:
- Add findAllOwnReviews() to collect all non-superseded matching reviews
- Loop over all old reviews in the supersede phase
- Add GetTimelineReviewCommentIDForReview() to find comment IDs by
review ID (fetches review body, matches in timeline by prefix)
- Each old review gets independently superseded and its inline comments
resolved
The old findOwnReview is kept for backward compat (tested, may be
useful as a utility).
Closes#27
After superseding an old review, resolves all its inline comments via
POST /pulls/comments/{id}/resolve. This clears unresolved conversation
markers from the PR timeline and diff view.
New API methods:
- ListReviewComments: paginated GET /repos/.../pulls/{n}/reviews/{id}/comments
- ResolveComment: POST /repos/.../pulls/comments/{id}/resolve
Behavior:
- Only resolves after successful supersede (gated on supersedeOK)
- Aggregates failures and logs at warn level
- Truncates error bodies to 256 bytes (security)
- Non-fatal: review still posts even if resolution fails
- Accept 204 No Content as success (idempotent operations)
- Truncate error response body to 256 bytes (prevent log leakage)
- Add unit tests for GetAuthenticatedUser and RequestReviewer
Closes#35
Before posting a review, the bot:
1. Discovers its own Gitea login via GET /user
2. Calls POST /requested_reviewers to add itself
This ensures the bot appears in the required-reviewers list without
manual configuration on the repo. The call is idempotent (no-op if
already requested).
Both failures are non-fatal (warn + continue) — the review still posts
even if the self-request fails.
Changes the order of operations:
1. POST new review (gets non-stale badge immediately)
2. PATCH old review with superseded message linking to the new one
This gives the superseded comment a clickable link to the current
review, making navigation between review iterations easy.
buildSupersededBody now accepts a newReviewURL parameter.
The strict authorship check compared reviewer-name to User.Login which
could mismatch. The sentinel is already role-specific (e.g.
<!-- review-bot:sonnet -->) and Gitea's API blocks editing others'
comments (403). Defense-in-depth via login comparison is unnecessary
complexity that introduced a bug. Removed.
Closes#34
- Remove reviewUnchanged() skip logic — every push gets a fresh review
- Remove edit-in-place (PATCH same body) — always POST new
- Supersede old review: PATCH with struck-through banner + collapsed
original body in <details> for historical reference
- Add commit footer to every review: 'Evaluated against <sha>'
- Remove --update-existing flag (no longer needed)
- Add CommitID field to Review struct
- Add TestBuildSupersededBody tests
- Add --log-format flag (text/json) and --verbosity flag (debug/info/warn/error)
- Replace all log.Printf with slog.Info/Debug/Warn with structured key-value attrs
- Replace all log.Fatalf with slog.Error + os.Exit(1)
- Convert gitea/client.go warnings to slog.Warn
- Add comprehensive tests for logger initialization and level filtering
Closes#23
Partially addresses #32
Add a Runner Requirements section to the README documenting that
the composite action needs python3, sha256sum, and curl on the
runner. All are pre-installed on ubuntu-* runners but custom
images need to provide them.
Closes#12
- URL-encode filename in release upload query param (MINOR)
- Truncate APIError.Body to 200 chars in Error() to avoid leaking
verbose server responses into logs (NIT)
- Add APIError type with StatusCode field so callers can inspect HTTP
status codes from Gitea API responses
- Add IsNotFound helper for ergonomic 404 checks
- GetAllFilesInPath now only falls back to single-file fetch on 404;
all other errors (auth failures, server errors, rate limits) propagate
- Release workflow asset uploads are now idempotent: existing assets
with the same name are deleted before re-upload on workflow re-runs
Closes#8Closes#10
The security-review-bot Gitea user now has its own token. This
completes the token separation so each reviewer role posts under
its own identity, enabling native Gitea multi-reviewer blocking.
When hasSharedToken() detects two roles sharing the same Gitea user,
the bot now skips ALL update logic (PATCH, supersede) and always POSTs
a fresh review. This prevents clobbering a sibling's review body or
state when misconfigured.
Tests now assert return values (true/false) rather than just verifying
no panic. Added additional test case for three-roles-same-user scenario.
Addresses review feedback: update logic and review state must not
interact with sibling reviews under the same user.
When two review-bot roles share the same Gitea user token (misconfiguration),
log a WARNING identifying which sibling is sharing. The bot continues normally
with its own honest verdict — no escalation, no deadlock. Operators see the
warning in CI logs and can fix the token setup.
Addresses Aaron's review feedback on #28: graceful degradation when someone
doesn't follow the separate-token deployment instructions.
Apply url.PathEscape to owner, repo, and sha path segments in all
methods that were previously interpolating raw values. Methods already
using PathEscape (ListReviews, DeleteReview, GetTimelineReviewCommentID,
EditComment) are unchanged.
This eliminates an inconsistency flagged in PRs #17, #20, and #22 and
prevents potential path-injection bugs for names with special characters.
Closes#24
Explains the edit-in-place approach, state transition rules, worst-wins
escalation, and inline comment lifecycle. Includes a Mermaid state
diagram for visual reference.
1. First-run escalation regression (MAJOR): Add post-posting escalation
fallback. After posting APPROVED on first run, check if a sibling
from the same user has REQUEST_CHANGES — if so, mark ours as
superseded and re-post as REQUEST_CHANGES.
2. json.Marshal error handling (MINOR): Return error from EditComment
instead of ignoring it with blank identifier.
3. Redundant condition (NIT): Remove dead assignment in reviewUnchanged
where existingEvent was assigned from r.State then compared to itself.
Replace the delete-and-repost strategy with edit-in-place:
1. No existing review → POST new (first run)
2. Same state, same body → skip entirely (threads preserved)
3. Same state, body changed → PATCH body in place via timeline API
4. State change needed → PATCH old body to "Superseded", POST new
This preserves conversation threads on inline comments. Replies to
findings are never lost. The only time a new review is posted is on
first run or when the state transitions (APPROVED ↔ REQUEST_CHANGES).
New Gitea client methods:
- EditComment: PATCH /repos/{owner}/{repo}/issues/comments/{id}
- GetTimelineReviewCommentID: finds the comment ID for a review body
by scanning the issue timeline for the sentinel
Also simplifies shouldEscalate: removes the login parameter requirement
for pre-posting scenarios (uses findOwnReview to get login from existing
review instead).
Tests: findOwnReview (4 cases), EditComment (2 cases),
GetTimelineReviewCommentID (2 cases), shouldEscalate (8 cases updated).
Before posting, compare the new review body+event against the existing
review with the same sentinel. If identical, skip entirely — this
preserves conversation threads on inline comments and avoids
re-notifying reviewers for findings they already know about.
Only re-posts when findings actually change (fixed, new, or different).
Tests: 6 cases covering identical, different body, different state,
stale reviews, and different sentinels.
- Hunk headers without comma ("@@ -1 +1 @@") now parse correctly by
splitting on comma OR space instead of comma only
- Explicit skip for "\ No newline at end of file" lines (was already
safe but now documents intent)
- Tests added for both edge cases (TDD: tests written first, confirmed
failure, then fixed)
Addresses sonnet findings #1 and #2 from PR #26 review.
Findings that reference a file+line within the diff are now posted as
inline comments directly on that line, in addition to appearing in the
summary table. Findings outside the diff range stay in the body only.
Implementation:
- gitea/diff.go: ParseDiffNewLines extracts new-file line numbers from
each hunk in the unified diff
- gitea/client.go: PostReview accepts optional []ReviewComment with
path + new_position + body (omitempty when nil)
- cmd/review-bot/main.go: maps findings → inline comments when the line
exists in the diff, passes them to PostReview
Tests:
- diff parser: multi-hunk, new files, empty diff, boundary lines
- PostReview: with comments, nil comments (omitted from payload)
Moved validateReviewerName check to right after flag parsing. Previously
it ran after the LLM request completed — wasting an expensive API call
if the name was invalid.
Sonnet review finding #1.
Rodin
