[NIT] The Python SSRF check script is duplicated verbatim in two steps ('Determine version' and 'Install review-bot'). This is intentional (defense-in-depth re-check before each curl that sends ACTION_TOKEN) and is documented in the comments. The duplication is justified by the security goal, but a helper function or sourced script would reduce maintenance burden if the check logic ever needs to change.
[MINOR] Using init() to parse CIDRs works but is slightly unusual when the data is pure compile-time constants. A panic on bad CIDR in init() would be more conventional (see configuration.md 'When NOT to Use' for init()). The chosen pattern (record errors, verify in tests) is an intentional trade-off documented in the code, but it means a typo in blockedCIDRStrings silently produces a weaker denylist at runtime if TestBlockedCIDRsValid is never run (e.g., in a binary without tests). A startup panic would catch this more reliably. However, given the CIDR list is stable and tests are run in CI, this is acceptable.
[MINOR] isValidateError does not use errors.As — it only checks for a direct type assertion. This means wrapped *validateError values (e.g., fmt.Errorf("...: %w", &validateError{...})) would not be detected. Since validateURL only ever returns bare *validateError values (never wrapped), this is safe in the current implementation. However, it deviates from the error-handling pattern in error-handling.md which prescribes errors.As for type extraction. A future refactor that wraps these errors would silently break the code-path. Consider using errors.As(err, out) instead.
[MINOR] The safeDialContext creates a new net.Dialer{} on every call with no timeout configured. The outer http.Client has a 30s timeout, but the dial itself has no deadline beyond the context. In the original code (pre-PR), there was no DialContext either, so this is not a regression. However, the PLAN.md design snippet shows d := &net.Dialer{Timeout: 10 * time.Second} with an explicit timeout, but the implementation uses &net.Dialer{} (no timeout). This deviation from the design is minor since the context propagates cancellation, but it could allow a slow TCP connect to consume the full 30s.
[NIT] PLAN.md is committed to the repository. Planning documents are typically kept out of version control (or in a docs/ directory) since they describe intent rather than implementation. The checklist items are outdated (showing unchecked boxes for things that are now implemented). This is a repo hygiene concern, not a code quality issue.
[MINOR] 198.51.100.1 (TEST-NET-2, RFC5737) is a documentation/testing range that should arguably be blocked, not considered public. The comment acknowledges it as 'public-looking' but it is actually a reserved range per RFC5737. This won't cause a security issue in practice (no real server lives there) but the comment is misleading. Consider either blocking it or removing the comment that implies ambiguity.
[MINOR] The package declaration is package gitea (not package gitea_test), which means NewTestClient is compiled into the gitea package during tests, making it accessible to all test files in the package. The comment says 'MUST only be called from _test.go files' but there is no enforcement beyond convention. Since it calls WithUnsafeDialer() which is an exported method on *Client, a caller in a non-test file could also call client.WithUnsafeDialer() directly. The naming (WithUnsafeDialer) and doc comment are sufficient deterrents, but the export_test.go approach only limits availability to the test binary, not to individual callers within the package. This is acceptable Go convention (the stdlib uses the same pattern in net/http/export_test.go).
[MINOR] The Python one-liner for extracting BINARY_ASSET_ID uses shell variable interpolation inside the Python string: if a['name'] == '${BINARY}'. If BINARY ever contained a single quote, this would break the Python string literal. In practice BINARY is constructed as review-bot-${OS}-${ARCH} where OS and ARCH are validated to be linux/darwin and amd64/arm64 respectively, so there's no actual risk. But the pattern of injecting shell variables into heredoc Python is worth noting.
[NIT] The VERSION validation regex [/[:space:]] would pass a version like v1.0.0 (correct) but would also pass v1.0.0%2F (URL-encoded slash). Not a real security risk since the value is used in a URL path constructed with ${VERSION} in bash (not eval'd), but worth noting that the validation doesn't cover all URL-unsafe characters.
[NIT] The echo "::add-mask::${ACTION_TOKEN}" is called AFTER the token has already been used in curl commands (which log to stdout). The masking only affects subsequent log output, not the curl command output already emitted. This is fine — the issue would be if curl verbose mode (-v) were used, which it isn't (-sSf suppresses output). The mask placement is correct for protecting the env var in subsequent steps.
[MINOR] The description comment says 'Detect VCS host type using github.api_url context' but the actual detection check is if [ -n "$GITHUB_API_URL" ]. On Gitea runners where this variable might be set to an empty string vs. truly unset, the behavior is correct. However, the PR description says 'absent on Gitea runners' — if a Gitea runner ever sets github.api_url to a non-empty value for some other reason, it would be misclassified as GitHub. This is a minor documentation/reasoning gap; the implementation is practically correct for known runners.
[NIT] The warning message ::warning::inputs.vcs-url is ignored on GitHub/GHES runners (VCS_TYPE=github). Using github.server_url instead. uses an inline ${{ inputs.vcs-url }} check inside the shell script. Since inputs.vcs-url is expanded by the Actions template engine before the shell runs, this is fine. However, if the input contained shell metacharacters it could cause issues. Given the set -euo pipefail at the top and that this branch only executes when the value is non-empty (the -n test), this is low risk in practice.
[MINOR] The checksum grep pattern was changed from ^[[:xdigit:]]+ to ^[0-9a-f]+. The new pattern is slightly more restrictive (rejects uppercase hex), which is fine for sha256sum output (always lowercase) but may fail if shasum -a 256 on macOS produces uppercase hex output in some locales. In practice shasum on macOS also outputs lowercase, so this is low risk, but the comment explaining why the regex changed would help future maintainers.
[MINOR] The detection logic uses github.api_url being non-empty to identify GitHub/GHES runners. However, some Gitea deployments may also expose this context variable (Gitea has been gradually increasing GitHub Actions compatibility). The comment in the file header says it is 'absent on Gitea Actions runners' but this assumption may not hold for future Gitea versions. A belt-and-suspenders approach — also checking whether the URL contains 'github' or comparing against known Gitea API path shapes — might be more robust long-term. For now this is acceptable given the stated requirement, but worth noting.
[NIT] PLAN-125.md is a planning/design artifact committed to the repository. Typically, implementation plans live in issues/wikis rather than the codebase itself. Consider removing it before merge or moving it to the issue tracker.