rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

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
Conversation 6 Commits 2 Files Changed 7
+215 -26
rodin commented 2026-05-14 05:26:26 +00:00
Owner
Copy Link
Copy Source

Summary

Implements issue #125: rename GITEA_URL env var to generic VCS_URL for multi-host support.

Plan

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, the name is misleading.

Constraints

  • Must not break existing users using GITEA_URL — deprecated fallback with warning
  • CLI flag updated: --gitea-url → --vcs-url
  • Integration test env vars updated: INTEGRATION_GITEA_URL → INTEGRATION_VCS_URL
  • Action YAML input updated: gitea-url → vcs-url

Backward Compatibility

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 N/A (action version bump)
INTEGRATION_GITEA_URL INTEGRATION_VCS_URL N/A (test-only)

Deprecation Behavior

If GITEA_URL is set and VCS_URL is not, binary logs:

level=WARN msg="GITEA_URL is deprecated; rename the environment variable to VCS_URL"

If --gitea-url is used and --vcs-url is not, binary logs:

level=WARN msg="--gitea-url is deprecated; use --vcs-url instead"

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

Files Changed

File Change
cmd/review-bot/main.go New --vcs-url flag + VCS_URL env, deprecated --gitea-url / GITEA_URL fallback
.gitea/actions/review/action.yml Rename input gitea-url → vcs-url, env GITEA_URL → VCS_URL
.gitea/workflows/ci.yml Rename GITEA_URL → VCS_URL
cmd/review-bot/integration_test.go INTEGRATION_GITEA_URL → INTEGRATION_VCS_URL
integration_test.go Same
README.md Update CLI example and env var table

Progress

  • VCS_URL is read first; GITEA_URL is fallback with deprecation warning
  • --vcs-url flag is primary; --gitea-url is deprecated alias with warning
  • Error message references --vcs-url not --gitea-url
  • action.yml passes VCS_URL (not GITEA_URL) to the binary
  • ci.yml passes VCS_URL (not GITEA_URL) to the binary
  • README updated in CLI example and env var table
  • Integration tests use INTEGRATION_VCS_URL
  • go test ./... passes
  • go vet ./... passes
  • go build ./cmd/review-bot succeeds
## Summary Implements issue #125: rename `GITEA_URL` env var to generic `VCS_URL` for multi-host support. ## Plan ### 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, the name is misleading. ### Constraints - Must not break existing users using `GITEA_URL` — deprecated fallback with warning - CLI flag updated: `--gitea-url` → `--vcs-url` - Integration test env vars updated: `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` - Action YAML input updated: `gitea-url` → `vcs-url` ### Backward Compatibility | 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` | N/A (action version bump) | | `INTEGRATION_GITEA_URL` | `INTEGRATION_VCS_URL` | N/A (test-only) | ### Deprecation Behavior If `GITEA_URL` is set and `VCS_URL` is not, binary logs: ``` level=WARN msg="GITEA_URL is deprecated; rename the environment variable to VCS_URL" ``` If `--gitea-url` is used and `--vcs-url` is not, binary logs: ``` level=WARN msg="--gitea-url is deprecated; use --vcs-url instead" ``` ### 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 ## Files Changed | File | Change | |------|--------| | `cmd/review-bot/main.go` | New `--vcs-url` flag + `VCS_URL` env, deprecated `--gitea-url` / `GITEA_URL` fallback | | `.gitea/actions/review/action.yml` | Rename input `gitea-url` → `vcs-url`, env `GITEA_URL` → `VCS_URL` | | `.gitea/workflows/ci.yml` | Rename `GITEA_URL` → `VCS_URL` | | `cmd/review-bot/integration_test.go` | `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` | | `integration_test.go` | Same | | `README.md` | Update CLI example and env var table | ## Progress - [x] `VCS_URL` is read first; `GITEA_URL` is fallback with deprecation warning - [x] `--vcs-url` flag is primary; `--gitea-url` is deprecated alias with warning - [x] Error message references `--vcs-url` not `--gitea-url` - [x] `action.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary - [x] `ci.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary - [x] README updated in CLI example and env var table - [x] Integration tests use `INTEGRATION_VCS_URL` - [x] `go test ./...` passes - [x] `go vet ./...` passes - [x] `go build ./cmd/review-bot` succeeds
aweiker was assigned by rodin 2026-05-14 05:26:26 +00:00
rodin added the ready label 2026-05-14 05:27:09 +00:00
sonnet-review-bot approved these changes 2026-05-14 05:27:52 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit 4f70f423)

Sonnet Review

Summary

Clean, well-scoped rename of GITEA_URL to VCS_URL with proper backward-compatibility fallback. The implementation correctly defers the deprecated env var check until after flag.Parse() and setupLogger() so the warning routes through slog. All affected files (action YAML, CI workflow, integration tests, README) are consistently updated.

Findings

# Severity File Line Finding
1 [NIT] PLAN-125.md 1 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.
2 [NIT] README.md 299 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.
3 [NIT] cmd/review-bot/main.go 57 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.

Recommendation

APPROVE — Approve with minor follow-up suggestions. The core logic is correct: VCS_URL is read at flag registration time via os.Getenv, the GITEA_URL and --gitea-url fallbacks are checked post-parse after slog is initialized (so warnings are properly structured), and all callers of the URL variable are updated consistently. The only actionable item before merge is updating the CLI example in README.md to show --vcs-url instead of --gitea-url — the env var table was fixed but the example command block was missed. The PLAN-125.md file should ideally not be committed to the repo.

Review by sonnet

Evaluated against 4f70f423

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/126#pullrequestreview-3680) for up-to-date findings. <details><summary>Previous findings (commit 4f70f423)</summary> # Sonnet Review ## Summary Clean, well-scoped rename of GITEA_URL to VCS_URL with proper backward-compatibility fallback. The implementation correctly defers the deprecated env var check until after `flag.Parse()` and `setupLogger()` so the warning routes through slog. All affected files (action YAML, CI workflow, integration tests, README) are consistently updated. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `PLAN-125.md` | 1 | 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`. | | 2 | [NIT] | `README.md` | 299 | 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. | | 3 | [NIT] | `cmd/review-bot/main.go` | 57 | `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. | ## Recommendation **APPROVE** — Approve with minor follow-up suggestions. The core logic is correct: VCS_URL is read at flag registration time via `os.Getenv`, the GITEA_URL and --gitea-url fallbacks are checked post-parse after slog is initialized (so warnings are properly structured), and all callers of the URL variable are updated consistently. The only actionable item before merge is updating the CLI example in README.md to show `--vcs-url` instead of `--gitea-url` — the env var table was fixed but the example command block was missed. The PLAN-125.md file should ideally not be committed to the repo. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 4f70f423* </details> <!-- review-bot:sonnet -->
PLAN-125.md Outdated
@@ -0,0 +1,175 @@
# Plan: Issue #125 — Rename GITEA_URL → VCS_URL
sonnet-review-bot commented 2026-05-14 05:27:52 +00:00
First-time contributor
Copy Link
Copy Source

[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`.
README.md
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
sonnet-review-bot commented 2026-05-14 05:27:52 +00:00
First-time contributor
Copy Link
Copy Source

[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.
cmd/review-bot/main.go Outdated
@@ -55,3 +55,3 @@
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)")
sonnet-review-bot commented 2026-05-14 05:27:52 +00:00
First-time contributor
Copy Link
Copy Source

[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.
gpt-review-bot approved these changes 2026-05-14 05:28:25 +00:00
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit 4f70f423)

Gpt Review

Summary

The changes correctly introduce VCS_URL/--vcs-url with proper deprecation fallbacks for GITEA_URL/--gitea-url, update the composite action and CI workflow, and adjust integration tests accordingly. Implementation order ensures deprecation warnings go through slog and precedence rules are handled correctly.

Findings

# Severity File Line Finding
1 [MINOR] README.md 299 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.

Recommendation

APPROVE — Overall, this PR cleanly implements the migration to VCS_URL/--vcs-url while maintaining backward compatibility and clear deprecation warnings. The precedence behavior (new flag/env wins over deprecated ones) is correct, and action/workflow updates align with the code. Please update the README’s CLI Usage example to reflect --vcs-url to keep documentation consistent with the new interface. Otherwise, the changes look solid and ready to merge.

Review by gpt

Evaluated against 4f70f423

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/126#pullrequestreview-3683) for up-to-date findings. <details><summary>Previous findings (commit 4f70f423)</summary> # Gpt Review ## Summary The changes correctly introduce VCS_URL/--vcs-url with proper deprecation fallbacks for GITEA_URL/--gitea-url, update the composite action and CI workflow, and adjust integration tests accordingly. Implementation order ensures deprecation warnings go through slog and precedence rules are handled correctly. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `README.md` | 299 | 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. | ## Recommendation **APPROVE** — Overall, this PR cleanly implements the migration to VCS_URL/--vcs-url while maintaining backward compatibility and clear deprecation warnings. The precedence behavior (new flag/env wins over deprecated ones) is correct, and action/workflow updates align with the code. Please update the README’s CLI Usage example to reflect --vcs-url to keep documentation consistent with the new interface. Otherwise, the changes look solid and ready to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 4f70f423* </details> <!-- review-bot:gpt -->
README.md
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
gpt-review-bot commented 2026-05-14 05:28:25 +00:00
First-time contributor
Copy Link
Copy Source

[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.
security-review-bot requested review from security-review-bot 2026-05-14 05:29:25 +00:00
security-review-bot approved these changes 2026-05-14 05:29:25 +00:00
Dismissed
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Original review

Superseded — see current review for up-to-date findings.

Previous findings (commit 4f70f423)

Security Review

Summary

The changes implement a backward-compatible rename of GITEA_URL/--gitea-url to VCS_URL/--vcs-url and update related action/workflow/test references. CI passed and no exploitable vulnerabilities were introduced by this diff.

Findings

# Severity File Line Finding
1 [MINOR] .gitea/actions/review/action.yml 112 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.
2 [MINOR] cmd/review-bot/main.go 92 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.

Recommendation

APPROVE — Approve the PR. The rename to VCS_URL/--vcs-url with deprecation fallback is implemented cleanly and does not introduce security issues. For future hardening, consider validating the VCS_URL to require HTTPS, and in the composite action add input validation/sanitization for repo and vcs-url to prevent potential command substitution in bash if those values were ever influenced by untrusted sources.

Review by security

Evaluated against 4f70f423

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/126#pullrequestreview-3682) for up-to-date findings. <details><summary>Previous findings (commit 4f70f423)</summary> # Security Review ## Summary The changes implement a backward-compatible rename of GITEA_URL/--gitea-url to VCS_URL/--vcs-url and update related action/workflow/test references. CI passed and no exploitable vulnerabilities were introduced by this diff. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `.gitea/actions/review/action.yml` | 112 | 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. | | 2 | [MINOR] | `cmd/review-bot/main.go` | 92 | 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. | ## Recommendation **APPROVE** — Approve the PR. The rename to VCS_URL/--vcs-url with deprecation fallback is implemented cleanly and does not introduce security issues. For future hardening, consider validating the VCS_URL to require HTTPS, and in the composite action add input validation/sanitization for repo and vcs-url to prevent potential command substitution in bash if those values were ever influenced by untrusted sources. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 4f70f423* </details> <!-- review-bot:security -->
.gitea/actions/review/action.yml
@@ -112,10 +112,10 @@ runs:
id: version
security-review-bot commented 2026-05-14 05:29:25 +00:00
Collaborator
Copy Link
Copy Source

[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.
security-review-bot marked this conversation as resolved
cmd/review-bot/main.go
@@ -91,12 +92,24 @@ func main() {
security-review-bot commented 2026-05-14 05:29:25 +00:00
Collaborator
Copy Link
Copy Source

[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.
security-review-bot marked this conversation as resolved
rodin removed the ready label 2026-05-14 05:40:07 +00:00
aweiker was unassigned by rodin 2026-05-14 05:40:07 +00:00
rodin self-assigned this 2026-05-14 05:40:07 +00:00
rodin added 2 commits 2026-05-14 06:00:47 +00:00
feat(#125): rename GITEA_URL to VCS_URL with deprecated fallback 5f7ffab487 
- 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.
fix: remove trailing whitespace in action.yml
PR Ready Gate / clear-labels (pull_request) Successful in 1s
Details
CI / test (pull_request) Successful in 15s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m17s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
Details
b80a1517ed
rodin force-pushed issue-125 from 4f70f423b4 to b80a1517ed 2026-05-14 06:00:47 +00:00 Compare
rodin added the readyself-reviewed labels 2026-05-14 06:00:51 +00:00
rodin removed their assignment 2026-05-14 06:00:58 +00:00
aweiker was assigned by rodin 2026-05-14 06:00:58 +00:00
sonnet-review-bot approved these changes 2026-05-14 06:01:27 +00:00
sonnet-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Sonnet Review

Summary

Clean, well-executed rename from GITEA_URL/--gitea-url to VCS_URL/--vcs-url with proper backward compatibility. The implementation follows the design outlined in the plan, CI passes, and the deprecation warnings are correctly placed post-flag-parse so they go through slog. No functional regressions identified.

Findings

# Severity File Line Finding
1 [NIT] PLAN-125.md 1 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.
2 [NIT] README.md 299 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.
3 [NIT] cmd/review-bot/main.go 57 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).

Recommendation

APPROVE — Approve with minor nits. The core implementation is correct: backward compatibility is preserved via post-parse env var and flag fallbacks, slog warnings are emitted at the right time (after setupLogger), all references to the old names are updated in CI config and action YAML, and the error message now references --vcs-url. The only actionable item worth fixing before merge is the stale --gitea-url in the README CLI usage example — users copy-pasting it will get a deprecation warning on every run. The PLAN-125.md file should also be cleaned up from the repo. Neither is a blocker, but both are worth addressing.

Review by sonnet

Evaluated against b80a1517

# Sonnet Review ## Summary Clean, well-executed rename from GITEA_URL/--gitea-url to VCS_URL/--vcs-url with proper backward compatibility. The implementation follows the design outlined in the plan, CI passes, and the deprecation warnings are correctly placed post-flag-parse so they go through slog. No functional regressions identified. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `PLAN-125.md` | 1 | 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. | | 2 | [NIT] | `README.md` | 299 | 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. | | 3 | [NIT] | `cmd/review-bot/main.go` | 57 | 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). | ## Recommendation **APPROVE** — Approve with minor nits. The core implementation is correct: backward compatibility is preserved via post-parse env var and flag fallbacks, slog warnings are emitted at the right time (after setupLogger), all references to the old names are updated in CI config and action YAML, and the error message now references --vcs-url. The only actionable item worth fixing before merge is the stale `--gitea-url` in the README CLI usage example — users copy-pasting it will get a deprecation warning on every run. The PLAN-125.md file should also be cleaned up from the repo. Neither is a blocker, but both are worth addressing. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against b80a1517*
PLAN-125.md
@@ -0,0 +1,175 @@
# Plan: Issue #125 — Rename GITEA_URL → VCS_URL
sonnet-review-bot commented 2026-05-14 06:01:27 +00:00
First-time contributor
Copy Link
Copy Source

[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.
README.md
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
sonnet-review-bot commented 2026-05-14 06:01:27 +00:00
First-time contributor
Copy Link
Copy Source

[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.
cmd/review-bot/main.go
@@ -55,3 +55,3 @@
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)")
sonnet-review-bot commented 2026-05-14 06:01:27 +00:00
First-time contributor
Copy Link
Copy Source

[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).
security-review-bot requested review from security-review-bot 2026-05-14 06:02:20 +00:00
security-review-bot approved these changes 2026-05-14 06:02:21 +00:00
security-review-bot left a comment
Collaborator
Copy Link
Copy Source

Security Review

Summary

Changes cleanly rename GITEA_URL/--gitea-url to VCS_URL/--vcs-url with proper deprecation warnings and precedence handling. No new security-relevant risks are introduced, and CI has passed.

Recommendation

APPROVE — The migration logic in main.go correctly prioritizes VCS_URL and --vcs-url while providing clear deprecation warnings for legacy env/flag usage. The composite action and CI workflow updates are consistent with the new naming and do not introduce additional security exposure beyond existing behavior. Consider, in future hardening, validating external download URLs and signature verification for binaries in the action, but this is outside the scope of the renaming change. Overall, the changes are secure and backward-compatible.

Review by security

Evaluated against b80a1517

# Security Review ## Summary Changes cleanly rename GITEA_URL/--gitea-url to VCS_URL/--vcs-url with proper deprecation warnings and precedence handling. No new security-relevant risks are introduced, and CI has passed. ## Recommendation **APPROVE** — The migration logic in main.go correctly prioritizes VCS_URL and --vcs-url while providing clear deprecation warnings for legacy env/flag usage. The composite action and CI workflow updates are consistent with the new naming and do not introduce additional security exposure beyond existing behavior. Consider, in future hardening, validating external download URLs and signature verification for binaries in the action, but this is outside the scope of the renaming change. Overall, the changes are secure and backward-compatible. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against b80a1517*
gpt-review-bot requested changes 2026-05-14 06:02:27 +00:00
gpt-review-bot left a comment
First-time contributor
Copy Link
Copy Source

Gpt Review

Summary

Overall the refactor from GITEA_URL/--gitea-url to VCS_URL/--vcs-url is implemented cleanly across the CLI, action, workflow, and tests, with appropriate deprecation warnings. However, there is a precedence bug in main.go where the deprecated --gitea-url flag does not override the GITEA_URL env var, which is surprising and contrary to typical flag-over-env precedence.

Findings

# Severity File Line Finding
1 [MAJOR] cmd/review-bot/main.go 92 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.

Recommendation

REQUEST_CHANGES — Address the precedence of configuration sources so that explicit flags override environment variables, even for the deprecated alias. A simple fix is to reorder the post-parse fallback logic to check the deprecated flag before the deprecated env var, e.g.:

  1. After flag.Parse and logger setup, if *vcsURL is empty and *giteaURLAlias is non-empty, set *vcsURL to the alias and warn.
  2. If still empty, then check VCS_URL env (already handled by the flag default) and finally GITEA_URL env, warning on use.

Concretely, replace the block starting at line 92 with:

if *vcsURL == "" && *giteaURLAlias != "" {
slog.Warn("--gitea-url is deprecated; use --vcs-url instead")
*vcsURL = *giteaURLAlias
}
if *vcsURL == "" {
if v := os.Getenv("GITEA_URL"); v != "" {
slog.Warn("GITEA_URL is deprecated; rename the environment variable to VCS_URL")
*vcsURL = v
}
}

This yields a consistent precedence: --vcs-url > --gitea-url > VCS_URL env > GITEA_URL env. After this change, re-run tests and verify the deprecation warnings still surface as intended. Everything else in the PR (action/workflow env updates, integration test env var renames, README env table update) looks good.

Review by gpt

Evaluated against b80a1517

# Gpt Review ## Summary Overall the refactor from GITEA_URL/--gitea-url to VCS_URL/--vcs-url is implemented cleanly across the CLI, action, workflow, and tests, with appropriate deprecation warnings. However, there is a precedence bug in main.go where the deprecated --gitea-url flag does not override the GITEA_URL env var, which is surprising and contrary to typical flag-over-env precedence. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `cmd/review-bot/main.go` | 92 | 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. | ## Recommendation **REQUEST_CHANGES** — Address the precedence of configuration sources so that explicit flags override environment variables, even for the deprecated alias. A simple fix is to reorder the post-parse fallback logic to check the deprecated flag before the deprecated env var, e.g.: 1) After flag.Parse and logger setup, if *vcsURL is empty and *giteaURLAlias is non-empty, set *vcsURL to the alias and warn. 2) If still empty, then check VCS_URL env (already handled by the flag default) and finally GITEA_URL env, warning on use. Concretely, replace the block starting at line 92 with: if *vcsURL == "" && *giteaURLAlias != "" { slog.Warn("--gitea-url is deprecated; use --vcs-url instead") *vcsURL = *giteaURLAlias } if *vcsURL == "" { if v := os.Getenv("GITEA_URL"); v != "" { slog.Warn("GITEA_URL is deprecated; rename the environment variable to VCS_URL") *vcsURL = v } } This yields a consistent precedence: --vcs-url > --gitea-url > VCS_URL env > GITEA_URL env. After this change, re-run tests and verify the deprecation warnings still surface as intended. Everything else in the PR (action/workflow env updates, integration test env var renames, README env table update) looks good. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against b80a1517*
cmd/review-bot/main.go
@@ -91,12 +92,24 @@ func main() {
gpt-review-bot commented 2026-05-14 06:02:27 +00:00
First-time contributor
Copy Link
Copy Source

[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.
rodin added wip and removed ready labels 2026-05-14 06:11:39 +00:00
aweiker was unassigned by rodin 2026-05-14 06:11:39 +00:00
rodin self-assigned this 2026-05-14 06:11:39 +00:00
rodin added ready and removed self-reviewedwip labels 2026-05-14 06:18:29 +00:00
rodin removed their assignment 2026-05-14 06:18:34 +00:00
aweiker was assigned by rodin 2026-05-14 06:18:35 +00:00
rodin merged commit c56f5fec52 into main 2026-05-14 06:38:54 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
sonnet-review-bot
security-review-bot
gpt-review-bot
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees aweiker
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#126
Delete Branch "issue-125"

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?