feat(#125): rename GITEA_URL to VCS_URL with deprecated fallback #126

Merged
rodin merged 2 commits from issue-125 into main 2026-05-14 06:38:54 +00:00
7 changed files with 215 additions and 26 deletions
+8 -8
View File
@@ -6,8 +6,8 @@ name: 'AI Code Review'
description: 'Run AI-powered code review on a pull request using review-bot'
inputs:
gitea-url:
description: 'Gitea instance URL (defaults to server_url)'
vcs-url:
description: 'VCS server URL (defaults to server_url)'
required: false
default: ''
repo:
@@ -112,10 +112,10 @@ runs:
id: version
security-review-bot marked this conversation as resolved
Review

[MINOR] Inputs (repo, vcs-url) are used directly in double-quoted bash strings for curl URLs. If these inputs contained command substitution syntax like (...), bash would execute it even within quotes. Although typically controlled by trusted workflow authors, consider validating inputs against an allowlist (e.g., owner/name pattern) or sanitizing to strip , backticks, and parentheses to reduce risk.

**[MINOR]** Inputs (repo, vcs-url) are used directly in double-quoted bash strings for curl URLs. If these inputs contained command substitution syntax like $(...), bash would execute it even within quotes. Although typically controlled by trusted workflow authors, consider validating inputs against an allowlist (e.g., owner/name pattern) or sanitizing to strip $, backticks, and parentheses to reduce risk.
shell: bash
run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
BASE_URL="${{ inputs.vcs-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \
VERSION=$(curl -sSf "${BASE_URL}/api/v1/repos/${REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2
@@ -161,14 +161,14 @@ runs:
if: steps.cache.outputs.cache-hit != 'true'
shell: bash
run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
BASE_URL="${{ inputs.vcs-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
curl -sSfL "${BASE_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
curl -sSfL "${BASE_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum
@@ -198,7 +198,7 @@ runs:
- name: Run review
shell: bash
env:
GITEA_URL: ${{ inputs.gitea-url || github.server_url }}
VCS_URL: ${{ inputs.vcs-url || github.server_url }}
GITEA_REPO: ${{ inputs.repo || github.repository }}
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
+1 -1
View File
@@ -49,7 +49,7 @@ jobs:
- run: go build -o review-bot ./cmd/review-bot
- name: Run ${{ matrix.name }} review
env:
GITEA_URL: ${{ github.server_url }}
VCS_URL: ${{ github.server_url }}
GITEA_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
+175
View File
@@ -0,0 +1,175 @@
# Plan: Issue #125 — Rename GITEA_URL → VCS_URL
Outdated
Review

[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]** 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`.
Review

[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]** 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.
## Problem
The `GITEA_URL` environment variable (and `--gitea-url` flag) implies the binary only works with Gitea.
Now that review-bot supports both Gitea and GitHub/GHES, this name is misleading.
Renaming to `VCS_URL` makes the binary platform-agnostic in its interface.
## Constraints
- Must not break existing users who already use `GITEA_URL` — need a fallback
- The CLI flag `--gitea-url` should also be updated to `--vcs-url` for consistency
- `INTEGRATION_GITEA_URL` in integration tests is a test-only env var, not the binary's interface; but should be updated for clarity
- The action YAML uses `GITEA_URL` as an internal shell variable in bash scripts — distinct from the env var passed to the binary
- All changes must compile and pass existing tests
## Files Affected
### Binary / Go source
| File | Change |
|------|--------|
| `cmd/review-bot/main.go` | Rename `--gitea-url``--vcs-url`, add `VCS_URL` as primary, keep `GITEA_URL` fallback |
| `cmd/review-bot/integration_test.go` | Rename `INTEGRATION_GITEA_URL``INTEGRATION_VCS_URL` (test-only, no external compat concern) |
| `integration_test.go` | Same — rename `INTEGRATION_GITEA_URL``INTEGRATION_VCS_URL` |
### Action YAML
| File | Change |
|------|--------|
| `.gitea/actions/review/action.yml` | Rename input `gitea-url``vcs-url`; update env var passed to binary: `VCS_URL` instead of `GITEA_URL`; keep internal bash var as `GITEA_URL` (only used for release download, not passed to binary) |
| `.gitea/workflows/ci.yml` | Rename `GITEA_URL` env var to `VCS_URL` in Run review step |
### Documentation
| File | Change |
|------|--------|
| `README.md` | Update CLI example, env var table entry |
## Proposed Approach
### 1. Backward-compatible env var lookup in main.go
Replace:
```go
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
```
With:
```go
giteaURL := flag.String("vcs-url", envOrDefaultFallback("VCS_URL", "GITEA_URL", ""), "VCS server URL (e.g. https://gitea.example.com)")
```
Add a helper:
```go
// envOrDefaultFallback reads primary env var; if empty, falls back to deprecated env var.
func envOrDefaultFallback(primary, deprecated, defaultVal string) string {
if v := os.Getenv(primary); v != "" {
return v
}
if v := os.Getenv(deprecated); v != "" {
slog.Warn("deprecated env var in use; rename to " + primary, "old", deprecated, "new", primary)
return v
}
return defaultVal
}
```
**Note:** This must be called AFTER `setupLogger` conceptually, but the flag default is evaluated at flag registration time. Since `setupLogger` runs before `flag.Parse()`, the slog.Warn will print correctly at runtime. We use `log.Printf` as a fallback if this proves problematic.
Actually — flag defaults are evaluated at registration (line 57), before `setupLogger`. The warning won't go through slog. Two options:
- Use `log.Printf` for the deprecation warning (always visible)
- Move the fallback lookup to after `flag.Parse()`, checking if the parsed value is still empty
**Decision:** Move fallback to a post-parse check. This is cleaner:
```go
vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL")
flag.Parse()
// Backward compat: fall back to deprecated GITEA_URL
if *vcsURL == "" {
if v := os.Getenv("GITEA_URL"); v != "" {
slog.Warn("GITEA_URL is deprecated; use VCS_URL instead")
*vcsURL = v
}
}
```
This is clean, idiomatic, and the warning goes through slog correctly.
### 2. Keep `--gitea-url` as deprecated alias
Add a hidden flag for backward compat:
```go
giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url")
```
Post-parse:
```go
if *vcsURL == "" && *giteaURLAlias != "" {
slog.Warn("--gitea-url is deprecated; use --vcs-url instead")
*vcsURL = *giteaURLAlias
}
```
### 3. Internal variable rename
Rename `giteaURL` local variable → `vcsURL` throughout `main.go` for consistency.
### 4. Error message update
```go
fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n")
```
### 5. Action YAML changes
In `.gitea/actions/review/action.yml`:
- Input `gitea-url``vcs-url` (with same description, `required: false`, `default: ''`)
- Line 172: `GITEA_URL: ${{ inputs.gitea-url || github.server_url }}``VCS_URL: ${{ inputs.vcs-url || github.server_url }}`
- Lines 115, 140: internal bash vars `GITEA_URL=` are used for downloading binaries — NOT passed to the review-bot binary. Leave them as internal bash vars (they're scope-local in bash). These could be renamed to `SERVER_URL` or `BASE_URL` for local clarity, but renaming them isn't strictly required.
In `.gitea/workflows/ci.yml`:
- Line 52: `GITEA_URL: ${{ github.server_url }}``VCS_URL: ${{ github.server_url }}`
### 6. Integration test updates
`INTEGRATION_GITEA_URL``INTEGRATION_VCS_URL` in both test files.
### 7. README
- CLI example: `--gitea-url``--vcs-url`
- Env var table: `GITEA_URL``VCS_URL`, add note about `GITEA_URL` fallback
## Backward Compatibility Summary
| Old | New | Fallback? |
|-----|-----|-----------|
| `GITEA_URL` env var | `VCS_URL` | ✅ with deprecation warning |
| `--gitea-url` flag | `--vcs-url` | ✅ with deprecation warning |
| `gitea-url` action input | `vcs-url` | ⚠️ No (action version bump handles this) |
| `INTEGRATION_GITEA_URL` | `INTEGRATION_VCS_URL` | N/A (test-only) |
## Error Cases
- Both `VCS_URL` and `GITEA_URL` set: `VCS_URL` wins (primary takes precedence)
- Both `--vcs-url` and `--gitea-url` provided: `--vcs-url` wins
- Neither set: existing "missing required flags" error unchanged
## Edge Cases
- `os.Getenv` returns "" for unset AND set-to-empty — consistent with existing behavior
- The `envOrDefault` helper is unchanged; we add `envOrDefaultFallback` for the one renamed var
## Testing Strategy
- Existing unit tests pass unchanged (they don't test env var parsing directly)
- Integration tests updated to use new env var name
- Manual: `GITEA_URL=https://example.com ./review-bot --repo x --pr 1 ...` should print deprecation warning and proceed
- Manual: `VCS_URL=https://example.com ./review-bot ...` should work silently
## Completion Checklist
1. `VCS_URL` is read first; `GITEA_URL` is fallback with deprecation warning
2. `--vcs-url` flag is primary; `--gitea-url` is deprecated alias with warning
3. Error message references `--vcs-url` not `--gitea-url`
4. `action.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary
5. `ci.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary
6. README updated in CLI example and env var table
7. Integration tests use `INTEGRATION_VCS_URL`
8. `go test ./...` passes
9. `go vet ./...` passes
10. `go build ./cmd/review-bot` succeeds
## Open Questions
- Should the CLI flag `--gitea-url` be completely hidden from `--help` or just deprecated with a note? The issue doesn't specify. Decision: keep it visible but add "(deprecated: use --vcs-url)" to the description.
- Should action.yml also add `gitea-url` as a deprecated input alias? The issue says "Update the action to pass the new env var name" — no mention of backward compat for the action input. Decision: rename only, no alias (action users pin a version anyway).
- The bash-internal `GITEA_URL` variable in action.yml scripts (used for release download, not passed to binary) — rename for clarity? Decision: yes, rename to `BASE_URL` to avoid confusion with the env var.
+1 -1
View File
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
Review

[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]** 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.
Review

[MINOR] README’s Env Var table reflects the new --vcs-url/VCS_URL mapping, but the earlier CLI Usage example still shows --gitea-url. Consider updating the CLI Usage section to use --vcs-url to avoid confusion.

**[MINOR]** README’s Env Var table reflects the new --vcs-url/VCS_URL mapping, but the earlier CLI Usage example still shows --gitea-url. Consider updating the CLI Usage section to use --vcs-url to avoid confusion.
Review

[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 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.
| Flag | Env Var |
|------|---------|
| `--gitea-url` | `GITEA_URL` |
| `--vcs-url` | `VCS_URL` (fallback: `GITEA_URL`) |
| `--repo` | `GITEA_REPO` |
| `--pr` | `PR_NUMBER` |
| `--reviewer-token` | `REVIEWER_TOKEN` |
+3 -3
View File
@@ -17,7 +17,7 @@ import (
// Integration test requires a running Gitea instance and LLM endpoint.
// Set environment variables:
//
// INTEGRATION_GITEA_URL - Gitea base URL
// INTEGRATION_VCS_URL - VCS base URL
// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
// INTEGRATION_PR_NUMBER - PR number to test against
@@ -25,7 +25,7 @@ import (
// INTEGRATION_LLM_API_KEY - LLM API key
// INTEGRATION_LLM_MODEL - Model name
func TestIntegration_FullReviewFlow(t *testing.T) {
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
giteaURL := os.Getenv("INTEGRATION_VCS_URL")
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
@@ -104,7 +104,7 @@ func TestIntegration_FullReviewFlow(t *testing.T) {
}
func TestIntegration_PostAndCleanup(t *testing.T) {
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
giteaURL := os.Getenv("INTEGRATION_VCS_URL")
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
+18 -5
View File
@@ -54,7 +54,8 @@ func main() {
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
// CLI flags
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL (e.g. https://gitea.example.com)")
Outdated
Review

[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.

**[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.
Review

[NIT] The --gitea-url flag description reads "Deprecated: use --vcs-url" which is useful, but per the plan's open question the decision was to keep it visible with a deprecation note. This is fine, but it could be slightly more helpful with the full flag name: "Deprecated: use --vcs-url instead" (matches the slog warning wording at line 101).

**[NIT]** The `--gitea-url` flag description reads `"Deprecated: use --vcs-url"` which is useful, but per the plan's open question the decision was to keep it visible with a deprecation note. This is fine, but it could be slightly more helpful with the full flag name: `"Deprecated: use --vcs-url instead"` (matches the slog warning wording at line 101).
giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url")
repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
@@ -91,12 +92,24 @@ func main() {
security-review-bot marked this conversation as resolved
Review

[MINOR] VCS_URL is accepted as-is without validation. As a defense-in-depth measure, validate that the URL uses HTTPS (and a sane host) to avoid accidentally sending reviewer tokens over insecure transport.

**[MINOR]** VCS_URL is accepted as-is without validation. As a defense-in-depth measure, validate that the URL uses HTTPS (and a sane host) to avoid accidentally sending reviewer tokens over insecure transport.
Review

[MAJOR] Precedence issue: when both deprecated --gitea-url flag and GITEA_URL env var are provided (with --vcs-url unset), the code prefers the env var over the flag. Typical CLI semantics are that flags override environment variables. Current fallback order is: --vcs-url > VCS_URL env > GITEA_URL env > --gitea-url. The alias flag should take precedence over env fallback.

**[MAJOR]** Precedence issue: when both deprecated --gitea-url flag and GITEA_URL env var are provided (with --vcs-url unset), the code prefers the env var over the flag. Typical CLI semantics are that flags override environment variables. Current fallback order is: --vcs-url > VCS_URL env > GITEA_URL env > --gitea-url. The alias flag should take precedence over env fallback.
slog.Info("review-bot starting", "version", version)
// Backward compatibility: fall back to deprecated env var / flag if VCS_URL / --vcs-url not set.
if *vcsURL == "" {
if v := os.Getenv("GITEA_URL"); v != "" {
slog.Warn("GITEA_URL is deprecated; rename the environment variable to VCS_URL")
*vcsURL = v
}
}
if *vcsURL == "" && *giteaURLAlias != "" {
slog.Warn("--gitea-url is deprecated; use --vcs-url instead")
*vcsURL = *giteaURLAlias
}
// Validate required fields
// For aicore provider, llm-base-url and llm-api-key are not required
isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
if *vcsURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n")
os.Exit(1)
}
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
@@ -139,7 +152,7 @@ func main() {
}
// Initialize clients
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
if *llmTemp < 0 || *llmTemp > 2 {
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
@@ -453,7 +466,7 @@ func main() {
// Supersede all old reviews with link to the new one
if len(oldReviews) > 0 {
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID)
for _, oldReview := range oldReviews {
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
if err != nil {
+9 -8
View File
@@ -16,16 +16,17 @@ import (
// Integration test requires a running Gitea instance and LLM endpoint.
// Set environment variables:
// INTEGRATION_GITEA_URL - Gitea base URL
// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
// INTEGRATION_PR_NUMBER - PR number to test against
// INTEGRATION_LLM_BASE_URL - LLM API base URL
// INTEGRATION_LLM_API_KEY - LLM API key
// INTEGRATION_LLM_MODEL - Model name
//
// INTEGRATION_VCS_URL - VCS base URL
// INTEGRATION_GITEA_TOKEN - Gitea API token with repo access
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
// INTEGRATION_PR_NUMBER - PR number to test against
// INTEGRATION_LLM_BASE_URL - LLM API base URL
// INTEGRATION_LLM_API_KEY - LLM API key
// INTEGRATION_LLM_MODEL - Model name
func TestIntegration_FullReviewFlow(t *testing.T) {
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
giteaURL := os.Getenv("INTEGRATION_VCS_URL")
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")