[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.
[NIT] The CLI usage example in README.md still shows --gitea-url (not --vcs-url). The env var table was updated but the example command block further up in the document was not changed. Users following the example will get the deprecated flag warning.
[NIT] The error message for unsupported OS re-runs $(uname -s) (uppercased original) rather than printing $OS_RAW (already computed). This is intentional and arguably better since it shows the raw value the user's system returned, but it adds a second uname fork. Cosmetic only.
[NIT] The OS case statement lowercases uname -s output via tr into OS_RAW, then matches linux and darwin and assigns identical strings. The tr and case together are correct but slightly redundant — you could match Linux and Darwin directly without tr. Not a bug, just minor ceremony. Current approach is defensively safe and readable.
[NIT] The checksum regex [[:xdigit:]]+[[:space:]]+\*?${BINARY}$ does not anchor the hex portion to the start of the line (^). A line like somejunk abc123 review-bot-linux-arm64 would not match (awk would still grab field 1), but the intent of exact matching is clearer with ^[[:xdigit:]]. Minor defensive concern only.
[NIT] The error message for unsupported OS re-invokes $(uname -s) (uppercase) after already computing `OS_RAW=$(uname -s
[NIT] PLAN-125.md is a planning artifact committed to the repository. Planning docs typically don't belong in version control alongside source code; they belong in the issue tracker or a wiki. Consider removing it before merge or adding *.md plan files to .gitignore.
[NIT] The CLI Usage example (around line 253 in README.md) still shows --gitea-url in the code block. The env var table was updated correctly, but the example command itself was not changed to --vcs-url. Users reading the quick-start example will see the deprecated flag.
[NIT] giteaURLAlias flag uses description "Deprecated: use --vcs-url" but the PR description and plan say it should be visible with the deprecation note. This is fine, though the description could be slightly more informative: "Deprecated: use --vcs-url instead" matches the slog warning text for consistency.
[MINOR] The github.api_url expression is interpolated directly into the shell variable: GITHUB_API_URL="${{ github.api_url }}" — this is safe only because github.api_url is a trusted platform-provided value (not user input). The comment explaining this exists for the API_URL construction but is absent at the point where GITHUB_API_URL is first assigned. A brief inline comment here would reinforce the security boundary, especially since the same pattern appears in the Install step at line 302 without explanation.
[MINOR] The SERVER_URL validation (https-only, no whitespace) is only applied when VCS_TYPE=gitea. On the GitHub path, github.server_url is trusted implicitly, which is correct. However if somehow github.server_url were empty (e.g., a misconfigured runner), SERVER_URL would be an empty string and later steps would silently construct malformed URLs. A guard like if [ -z "$SERVER_URL" ]; then exit 1; fi after the assignment would make failures explicit.
[NIT] The curl download for the binary uses -sSfL (follows redirects) on the GitHub path, but the comment in the Gitea path explicitly explains why -L is omitted (to prevent header forwarding on CDN redirect). The same security rationale should be documented for the GitHub path to explain why -L is acceptable there (GitHub S3 redirects don't receive the Authorization header because it's stripped on redirect by curl's default behavior for cross-origin redirects, but this isn't documented).
[MINOR] The BINARY variable (review-bot-linux-amd64) is embedded directly in the Python here-doc via shell interpolation (a['name'] == '${BINARY}'). While BINARY is a hardcoded constant in this script (not user input), the pattern conflates shell and Python variable scopes in a subtle way. A minor improvement would be to pass it as a Python argument (sys.argv[1]) or use os.environ, making the boundary explicit. Not a security issue here, but a readability/maintainability nit.
[NIT] The Python inline script for extracting BINARY_ASSET_ID uses sys.exit(1) in the else clause of the for loop, which will make curl's output silently disappear on failure. Combined with the if [ -z "$BINARY_ASSET_ID" ] check that follows, this creates a confusing double-check (the python exits 1 before the shell check fires). Either rely solely on the shell empty-check (remove sys.exit(1)) or rely solely on python's exit code — mixing both is redundant.
[NIT] The Run review step still uses GITEA_URL as the env var name even though this action now supports GitHub/GHES. This works since review-bot reads GITEA_URL and this PR intentionally sets it to server_url (which is correct for both hosts), but the variable name is misleading. This is a pre-existing naming issue and acceptable to address in a follow-up.