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
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 rodin2026-05-14 05:26:26 +00:00
rodin
added the ready label 2026-05-14 05:27:09 +00:00
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.
~~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 -->
[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`.
[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.
[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
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.
~~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 -->
[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
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.
~~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 -->
[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
[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 rodin2026-05-14 05:40:07 +00:00
rodin
self-assigned this 2026-05-14 05:40:07 +00:00
- 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.
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.
# 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*
[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.
[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.
[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
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.
# 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*
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.:
After flag.Parse and logger setup, if *vcsURL is empty and *giteaURLAlias is non-empty, set *vcsURL to the alias and warn.
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.
# 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*
[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 rodin2026-05-14 06:11:39 +00:00
rodin
self-assigned this 2026-05-14 06:11:39 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
sonnet-review-bot
security-review-bot
gpt-review-bot
aweiker
Summary
Implements issue #125: rename
GITEA_URLenv var to genericVCS_URLfor multi-host support.Plan
Problem
The
GITEA_URLenvironment variable (and--gitea-urlflag) implies the binary only works with Gitea. Now that review-bot supports both Gitea and GitHub/GHES, the name is misleading.Constraints
GITEA_URL— deprecated fallback with warning--gitea-url→--vcs-urlINTEGRATION_GITEA_URL→INTEGRATION_VCS_URLgitea-url→vcs-urlBackward Compatibility
GITEA_URLenv varVCS_URL--gitea-urlflag--vcs-urlgitea-urlaction inputvcs-urlINTEGRATION_GITEA_URLINTEGRATION_VCS_URLDeprecation Behavior
If
GITEA_URLis set andVCS_URLis not, binary logs:If
--gitea-urlis used and--vcs-urlis not, binary logs:Error Cases
VCS_URLandGITEA_URLset:VCS_URLwins (primary takes precedence)--vcs-urland--gitea-urlprovided:--vcs-urlwinsFiles Changed
cmd/review-bot/main.go--vcs-urlflag +VCS_URLenv, deprecated--gitea-url/GITEA_URLfallback.gitea/actions/review/action.ymlgitea-url→vcs-url, envGITEA_URL→VCS_URL.gitea/workflows/ci.ymlGITEA_URL→VCS_URLcmd/review-bot/integration_test.goINTEGRATION_GITEA_URL→INTEGRATION_VCS_URLintegration_test.goREADME.mdProgress
VCS_URLis read first;GITEA_URLis fallback with deprecation warning--vcs-urlflag is primary;--gitea-urlis deprecated alias with warning--vcs-urlnot--gitea-urlaction.ymlpassesVCS_URL(notGITEA_URL) to the binaryci.ymlpassesVCS_URL(notGITEA_URL) to the binaryINTEGRATION_VCS_URLgo test ./...passesgo vet ./...passesgo build ./cmd/review-botsucceedsOriginal reviewSuperseded — 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()andsetupLogger()so the warning routes through slog. All affected files (action YAML, CI workflow, integration tests, README) are consistently updated.Findings
PLAN-125.md*.mdplan files to.gitignore.README.md--gitea-urlin 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.gogiteaURLAliasflag 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-urlinstead 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@@ -0,0 +1,175 @@# Plan: Issue #125 — Rename GITEA_URL → VCS_URL[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
*.mdplan files to.gitignore.@@ -299,7 +299,7 @@ All flags have environment variable equivalents:[NIT] The CLI Usage example (around line 253 in README.md) still shows
--gitea-urlin 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.@@ -55,3 +55,3 @@verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")// CLI flagsgiteaURL := 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)")[NIT]
giteaURLAliasflag 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.Original reviewSuperseded — 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
README.mdRecommendation
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@@ -299,7 +299,7 @@ All flags have environment variable equivalents:[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.
Original reviewSuperseded — 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
.gitea/actions/review/action.yml(...), 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.cmd/review-bot/main.goRecommendation
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@@ -112,10 +112,10 @@ runs:id: version[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.@@ -91,12 +92,24 @@ func main() {[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.
4f70f423b4tob80a1517edSonnet 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
PLAN-125.mdREADME.md--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--gitea-urlflag 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-urlin 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@@ -0,0 +1,175 @@# Plan: Issue #125 — Rename GITEA_URL → VCS_URL[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.
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:[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.@@ -55,3 +55,3 @@verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")// CLI flagsgiteaURL := 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)")[NIT] The
--gitea-urlflag 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
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
b80a1517Gpt 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
cmd/review-bot/main.goRecommendation
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.:
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@@ -91,12 +92,24 @@ func main() {[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.