fix(#120): detect VCS host for releases API and derive action-repo #122
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: rodin/review-bot#122
Reference in New Issue
Block a user
Delete Branch "issue-120"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes the composite action so it works correctly on GitHub Enterprise Server as well as Gitea. Closes #120.
Problem
The action hardcoded three Gitea-specific assumptions:
REPOdefaulted torodin/review-bot— should derive from context or be configurable/api/v1/repos/...) — GitHub uses/api/v3/repos/....giteaaction's "Run review" step usedGITEA_URL/GITEA_REPOenv vars — the review-bot binary expectsGITHUB_SERVER_URL/GITHUB_REPOSITORYFix
VCS detection: Check if
server_urlcontainsgitea→ use/api/v1/(Gitea). Otherwise → use/api/v3/(GitHub).Action repo default: Auto-select
rodin/review-boton Gitea orstrat/review-boton GitHub whenaction-repoinput is not set.New
action-repoinput: Allows explicit override for non-default setups. Defaults to empty (auto-detect).Env var alignment: Fixed
.giteaaction's "Run review" step to setGITHUB_SERVER_URL/GITHUB_REPOSITORYinstead ofGITEA_URL/GITEA_REPO.Self-test workflow: Added
.github/workflows/review.ymlonstrat/review-botto validate the fix end-to-end on GitHub.Backward Compatibility
Existing Gitea callers using default inputs see no change:
server_urlcontainsgitea→/api/v1/selectedaction-repoempty → defaults torodin/review-botChecklist
action-reporesolves torodin/review-botvia/api/v1/action-reporesolves tostrat/review-botvia/api/v3/action-repoinput overrides auto-detection.gitea"Run review" step now usesGITHUB_SERVER_URL/GITHUB_REPOSITORY.github/workflows/review.ymlOriginal reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3ac5e5dc)Sonnet Review
Summary
This PR correctly fixes VCS host detection for GitHub Enterprise Server vs Gitea by introducing API path selection and auto-deriving the action-repo default. The logic is clean, backward-compatible, and the two action files are kept in sync. CI passes.
Findings
.github/workflows/review.yml./.gitea/actions/review(line 32:uses: ./.gitea/actions/review) rather than./.github/actions/review. On GitHub the runner will look for local composite actions under.github/, not.gitea/. While GitHub Actions does allow resolving.gitea/paths in some environments, this is unexpected and could break on a standard GitHub or GHES runner that doesn't support the Gitea convention. The intent of this workflow is to validate the GitHub path, so it should reference./.github/actions/review..gitea/actions/review/action.ymlgrep -qi 'gitea'matches on the word 'gitea' anywhere in the server URL. This works for the known cases (e.g.https://gitea.example.com) but would misfire for a GitHub Enterprise Server instance with 'gitea' in its hostname, or fail for a self-hosted Gitea instance that doesn't include 'gitea' in its URL (e.g.https://code.mycompany.com). A more robust approach would be to attempt the Gitea API path and fall back, or to expose an explicitapi-versioninput. This is acceptable as a pragmatic heuristic for the stated use-cases but worth documenting as a known limitation..gitea/actions/review/action.yml.github/actions/review/action.ymlcopy is also intended for GitHub/GHES runners. The comment should be updated in that file to reflect its dual purpose.Recommendation
APPROVE — The core logic is correct and well-structured. The one actionable issue worth fixing before this is relied upon in production is the
uses: ./.gitea/actions/reviewreference in.github/workflows/review.yml— this is the self-test workflow meant to validate the GitHub path, yet it points at the.gitea/action copy. Change that reference to./.github/actions/review. The VCS detection heuristic limitation is worth a comment but not a blocker given the stated scope. Everything else is a nit.Review by sonnet
Evaluated against
3ac5e5dc@@ -117,0 +119,4 @@SERVER_URL="${{ inputs.gitea-url || github.server_url }}"# Detect VCS type: Gitea uses /api/v1/, GitHub uses /api/v3/if echo "$SERVER_URL" | grep -qi 'gitea'; thenAPI_BASE="${SERVER_URL}/api/v1"[MINOR] The VCS detection heuristic
grep -qi 'gitea'matches on the word 'gitea' anywhere in the server URL. This works for the known cases (e.g.https://gitea.example.com) but would misfire for a GitHub Enterprise Server instance with 'gitea' in its hostname, or fail for a self-hosted Gitea instance that doesn't include 'gitea' in its URL (e.g.https://code.mycompany.com). A more robust approach would be to attempt the Gitea API path and fall back, or to expose an explicitapi-versioninput. This is acceptable as a pragmatic heuristic for the stated use-cases but worth documenting as a known limitation.@@ -0,0 +29,4 @@with:reviewer-token: ${{ secrets[matrix.token_secret] }}reviewer-name: ${{ matrix.name }}llm-model: ${{ matrix.model }}[MINOR] The self-test workflow uses
./.gitea/actions/review(line 32:uses: ./.gitea/actions/review) rather than./.github/actions/review. On GitHub the runner will look for local composite actions under.github/, not.gitea/. While GitHub Actions does allow resolving.gitea/paths in some environments, this is unexpected and could break on a standard GitHub or GHES runner that doesn't support the Gitea convention. The intent of this workflow is to validate the GitHub path, so it should reference./.github/actions/review.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3ac5e5dc)Security Review
Summary
The composite actions introduce command injection risks by interpolating untrusted inputs into shell without validation, and they download and execute binaries from user-specified URLs without enforcing HTTPS or domain validation. CI passed, but these issues are exploitable in certain workflows and should be addressed.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.yml...via command substitution..github/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.ymlRecommendation
REQUEST_CHANGES — Harden the shell scripts to prevent command injection and unsafe downloads:
Addressing the above will eliminate command injection vectors and reduce the risk of executing untrusted binaries or being affected by MITM/SSRF. Once fixed, the PR can be approved.
Review by security
Evaluated against
3ac5e5dc@@ -112,10 +116,21 @@ runs:id: version[MINOR] No enforcement of HTTPS for SERVER_URL; allowing http or arbitrary schemes permits MITM and integrity compromise of the downloaded binary and checksum.
@@ -117,0 +127,4 @@fiACTION_REPO="${{ inputs.action-repo || '' }}"if [ -z "$ACTION_REPO" ]; thenACTION_REPO="$DEFAULT_ACTION_REPO"[MAJOR] Command injection risk: unvalidated input (ACTION_REPO) is interpolated into a shell command argument for curl ("${API_BASE}/repos/${ACTION_REPO}/..."). If ACTION_REPO contains command substitution like $(...) or backticks, it will be executed by the shell even within double quotes.
[MINOR] Potential SSRF/internal network access: SERVER_URL is user-configurable and used for server-side requests without allowlisting or validation, enabling connections to internal services from the runner.
[MINOR] Missing network timeouts/retries for curl may cause the job to hang or be susceptible to long delays; consider adding --max-time/--connect-timeout and bounded retries.
@@ -142,2 +157,4 @@SERVER_URL="${{ steps.version.outputs.server-url }}"ACTION_REPO="${{ steps.version.outputs.action-repo }}"VERSION="${{ steps.version.outputs.version }}"BINARY="review-bot-linux-amd64"[MAJOR] Command injection risk: unvalidated inputs (SERVER_URL and ACTION_REPO) are used in curl URLs ("${SERVER_URL}/${ACTION_REPO}/releases/download..."). Embedded $(...) or backticks in either value will trigger command substitution and arbitrary command execution.
@@ -112,10 +116,21 @@ runs:id: version[MINOR] No enforcement of HTTPS for SERVER_URL; permitting non-HTTPS allows downgrade/MITM when downloading and executing the binary.
@@ -117,0 +127,4 @@fiACTION_REPO="${{ inputs.action-repo || '' }}"if [ -z "$ACTION_REPO" ]; thenACTION_REPO="$DEFAULT_ACTION_REPO"[MAJOR] Command injection risk: unvalidated ACTION_REPO is expanded into curl URL ("${API_BASE}/repos/${ACTION_REPO}/...") and may execute $(...) or
...via command substitution.[MINOR] Potential SSRF/internal network access: SERVER_URL is used for server-side requests without validation or allowlisting, enabling connections to arbitrary hosts from the runner.
[MINOR] Missing network timeouts/retries for curl may cause hangs; add --max-time/--connect-timeout and safe retry logic.
@@ -142,2 +157,4 @@SERVER_URL="${{ steps.version.outputs.server-url }}"ACTION_REPO="${{ steps.version.outputs.action-repo }}"VERSION="${{ steps.version.outputs.version }}"BINARY="review-bot-linux-amd64"[MAJOR] Command injection risk: unvalidated SERVER_URL and ACTION_REPO are used in curl calls ("${SERVER_URL}/${ACTION_REPO}/releases/download...") allowing command substitution if inputs contain $(...) or backticks.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
3ac5e5dc)Gpt Review
Summary
Solid improvements to make the composite action work across Gitea and GitHub Enterprise by detecting API base, decoupling the action binary repo, and aligning env vars with the binary. CI passed and the two action files are kept in sync; overall changes look correct and pragmatic.
Findings
.gitea/actions/review/action.ymlgrep -qi 'gitea'on server_url). This will misclassify Gitea instances without 'gitea' in the hostname (e.g., code.example.com), breaking API calls by selecting /api/v3. Consider a more robust detection (e.g., explicit input, or probing a known endpoint) or an input to force API version..gitea/actions/review/action.ymlper_page=1rather thanlimit=1. While GitHub ignores unknown params and still returns releases, usingper_page=1would be more correct for GitHub and reduce payload size; you can keeplimitfor Gitea..github/actions/review/action.yml'gitea'substring in server_url may mis-detect some Gitea instances. Provide an explicit override or more robust probe to avoid false negatives..github/actions/review/action.ymlper_page=1for GitHub to explicitly limit results;limit=1is a Gitea parameter.Recommendation
APPROVE — The refactor to detect API base paths, add an explicit action-repo input, and align environment variables with the review-bot expectations is well done and should improve cross-platform behavior. CI passed and both composite action files are kept in sync, which is great. Before broader adoption, consider adding an explicit input to select the VCS/API type (or a more robust probe) to avoid mis-detecting Gitea instances whose hostnames do not contain 'gitea'. Also, for the GitHub API call, switch to
per_page=1to reduce payload size and be semantically correct. These are minor enhancements and do not block merge.Review by gpt
Evaluated against
3ac5e5dc@@ -117,0 +122,4 @@API_BASE="${SERVER_URL}/api/v1"DEFAULT_ACTION_REPO="rodin/review-bot"elseAPI_BASE="${SERVER_URL}/api/v3"[MINOR] VCS host detection relies on a simple substring match (
grep -qi 'gitea'on server_url). This will misclassify Gitea instances without 'gitea' in the hostname (e.g., code.example.com), breaking API calls by selecting /api/v3. Consider a more robust detection (e.g., explicit input, or probing a known endpoint) or an input to force API version.@@ -117,0 +122,4 @@API_BASE="${SERVER_URL}/api/v1"DEFAULT_ACTION_REPO="rodin/review-bot"elseAPI_BASE="${SERVER_URL}/api/v3"[MINOR] Same VCS detection heuristic issue as in the Gitea action: relying on
'gitea'substring in server_url may mis-detect some Gitea instances. Provide an explicit override or more robust probe to avoid false negatives.Sonnet Review
Summary
This PR fixes multi-VCS support by detecting whether the server is Gitea or GitHub and selecting the appropriate API path and default action repo. The changes are clean, consistent across both action files, and the critical env var rename (GITEA_URL/GITEA_REPO → GITHUB_SERVER_URL/GITHUB_REPOSITORY) is correct. CI passes.
Findings
.gitea/actions/review/action.ymlgrep -qi 'gitea'is hostname-based string matching. A GitHub Enterprise instance at a URL likehttps://github.example-gitea-internal.comwould incorrectly be detected as Gitea and use/api/v1/. This is an edge case, but since the input isgitea-url(user-supplied), it's possible a user passes a GHE URL that happens to contain 'gitea' in the domain. The approach is pragmatic and acceptable for the known use cases, but worth documenting or guarding..github/workflows/review.yml./.gitea/actions/review(the Gitea action path) even though it runs on GitHub. This means the GitHub runner will execute the.gitea/actions/review/action.ymlfile. This is intentional based on the comment ('Download binary from Gitea, post review to Gitea'), but it's slightly odd that the GitHub workflow uses the.giteaaction path — the two action files are now identical, so it works, but it could confuse future maintainers who expect.github/workflowsto use.github/actions..gitea/actions/review/action.ymlRecommendation
APPROVE — Approve. The core changes are correct and well-structured: VCS detection logic is consistent across both action files, the API path selection is sound for the documented use cases, the env var rename fixes the real bug, and the two action files are now kept in sync. The minor findings are edge cases that don't affect the stated requirements. The self-test workflow is a good addition. No blocking issues.
Review by sonnet
Evaluated against
db130781@@ -114,3 +118,2 @@run: |GITEA_URL="${{ inputs.gitea-url || github.server_url }}"REPO="${{ inputs.repo || 'rodin/review-bot' }}"SERVER_URL="${{ inputs.gitea-url || github.server_url }}"[MINOR] The VCS detection heuristic
grep -qi 'gitea'is hostname-based string matching. A GitHub Enterprise instance at a URL likehttps://github.example-gitea-internal.comwould incorrectly be detected as Gitea and use/api/v1/. This is an edge case, but since the input isgitea-url(user-supplied), it's possible a user passes a GHE URL that happens to contain 'gitea' in the domain. The approach is pragmatic and acceptable for the known use cases, but worth documenting or guarding.@@ -117,0 +125,4 @@API_BASE="${SERVER_URL}/api/v3"DEFAULT_ACTION_REPO="strat/review-bot"fiACTION_REPO="${{ inputs.action-repo || '' }}"[NIT]
ACTION_REPO="${{ inputs.action-repo || '' }}"— the|| ''is redundant since the input already hasdefault: ''. It's harmless but slightly noisy.@@ -0,0 +27,4 @@- name: Run ${{ matrix.name }} reviewuses: ./.gitea/actions/reviewwith:# Download binary from Gitea (releases live there, not on GHE)[MINOR] The self-test workflow uses
./.gitea/actions/review(the Gitea action path) even though it runs on GitHub. This means the GitHub runner will execute the.gitea/actions/review/action.ymlfile. This is intentional based on the comment ('Download binary from Gitea, post review to Gitea'), but it's slightly odd that the GitHub workflow uses the.giteaaction path — the two action files are now identical, so it works, but it could confuse future maintainers who expect.github/workflowsto use.github/actions.Security Review
Summary
Security-impacting changes are minimal and CI passed. The VCS host detection, repo derivation, and environment alignment look sound, and no exploitable vulnerabilities are introduced by the diff. A few defense-in-depth improvements are suggested.
Findings
.gitea/actions/review/action.yml.github/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.ymlRecommendation
APPROVE — Approve the PR. The changes correctly detect the VCS host to select the appropriate API path, separate the download repo from the review target, and align environment variables with the binary’s expectations, all without introducing new exploitable issues. For defense-in-depth, strengthen binary integrity verification (e.g., signed releases, allowlisting known repos, or pinning versions/hashes stored in the repo) and add explicit curl timeouts to avoid potential hangs.
Review by security
Evaluated against
db130781@@ -112,10 +116,21 @@ runs:id: version[NIT] Network requests (curl) do not specify explicit timeouts, which can cause long hangs under adverse network conditions. Consider adding --max-time or --connect-timeout to reduce DoS risk from slow/unresponsive endpoints.
@@ -137,14 +154,14 @@ runs:if: steps.cache.outputs.cache-hit != 'true'[MINOR] The binary integrity check relies on a checksums.txt downloaded from the same untrusted release source. If ACTION_REPO or SERVER_URL are misconfigured or attacker-controlled (via workflow inputs), a malicious binary and matching checksum could be served and executed with access to secrets. Consider verifying signatures, pinning to an allowlist of repos, or pinning versions/hashes from a trusted source.
@@ -112,10 +116,21 @@ runs:id: version[NIT] Network requests (curl) lack explicit timeouts. Adding timeouts (--max-time/--connect-timeout) can improve robustness and reduce the risk of workflow stalls.
@@ -137,14 +154,14 @@ runs:if: steps.cache.outputs.cache-hit != 'true'[MINOR] Same supply-chain concern as above: the checksum file is fetched from the same source as the binary, which does not provide strong integrity guarantees if inputs are misconfigured. Strengthen verification by using signed releases or a maintained allowlist of action repos and expected hashes.
Gpt Review
Summary
The changes improve platform compatibility by detecting the VCS host to select the correct API base, add an action-repo override, fix environment variable names expected by the binary, and include a self-test workflow. CI passed and the two composite actions are functionally aligned. A few minor robustness and documentation tweaks are suggested.
Findings
.gitea/actions/review/action.yml.gitea/actions/review/action.yml.github/actions/review/action.yml.github/actions/review/action.yml.github/actions/review/action.yml.github/actions/review/action.yml.github/workflows/review.ymlRecommendation
APPROVE — Overall, the PR cleanly generalizes the composite action to work across Gitea and GitHub by selecting the appropriate API base, allows an explicit action-repo override, and corrects environment variables passed to the review-bot binary. The self-test workflow is a good addition. CI is green and the two composite action files appear aligned. Before merging, consider minor improvements: make VCS detection more robust than a substring match, use 'per_page=1' for GitHub's releases API and optionally support Authorization for private repositories, and update a few comments/descriptions for clarity across platforms. These are non-blocking; the current changes look correct and maintain backward compatibility.
Review by gpt
Evaluated against
db130781@@ -112,10 +116,21 @@ runs:id: version[MINOR] VCS detection relies on checking if the server URL contains the substring 'gitea'. This is brittle (a Gitea instance may not include 'gitea' in the hostname). Consider a more robust detection or a configurable input (e.g., explicit vcs-type or probing /api/v1 vs /api/v3) with fallback.
@@ -117,0 +123,4 @@DEFAULT_ACTION_REPO="rodin/review-bot"elseAPI_BASE="${SERVER_URL}/api/v3"DEFAULT_ACTION_REPO="strat/review-bot"[MINOR] When querying GitHub's releases API, the parameter should be 'per_page=1' rather than 'limit=1'. While GitHub ignores unknown params and still returns results, updating this would be more correct and avoid relying on default pagination. Also consider supporting an Authorization header for private repos.
@@ -112,10 +116,21 @@ runs:id: version[MINOR] Same brittle VCS detection as the Gitea action: using 'grep -qi gitea' on the server URL may mis-detect non-standard Gitea hostnames. Consider a more robust detection or a user-provided override with fallback probing.
@@ -117,0 +123,4 @@DEFAULT_ACTION_REPO="rodin/review-bot"elseAPI_BASE="${SERVER_URL}/api/v3"DEFAULT_ACTION_REPO="strat/review-bot"[MINOR] Releases API call uses 'limit=1'; for GitHub v3 the canonical parameter is 'per_page=1'. Also consider adding optional Authorization to support private repos.
@@ -0,0 +24,4 @@model: gpt-5steps:- uses: actions/checkout@v4- name: Run ${{ matrix.name }} review[NIT] The workflow uses the composite action from '.gitea/actions/review'. Since an equivalent exists in '.github/actions/review' and both are meant to be identical, consider referencing the .github path here for consistency when running on GitHub.
Closing as duplicate of #121, which targets
maindirectly with the same fix.Pull request closed