Compare commits

...

12 Commits

Author SHA1 Message Date
Rodin b80a1517ed fix: remove trailing whitespace in action.yml
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 15s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m17s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
2026-05-13 23:00:35 -07:00
Rodin 5f7ffab487 feat(#125): rename GITEA_URL to VCS_URL with deprecated fallback
- 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.
2026-05-13 23:00:35 -07:00
aweiker f8b9d7d282 fix: portable checksum on darwin, anchor grep pattern
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 27s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m4s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m13s
CI / test (push) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
- sha256sum is not available on macOS; use shasum -a 256 on darwin.
  Select based on steps.version.outputs.os which is already computed.
  Fixes MAJOR finding from gpt-review-bot (PR #127 review).

- Anchor checksum grep with ^ to avoid matching on partial lines.
  Fixes MINOR finding from gpt-review-bot (PR #127 review).
2026-05-14 05:48:21 +00:00
aweiker 7a8fc166ec feat(action): derive binary name from uname for multi-arch support (#124)
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 26s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m12s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m33s
Previously the Install step hard-coded 'review-bot-linux-amd64'. This
fails on arm64 runners (Graviton, Apple Silicon) where uname -m returns
'aarch64' or 'arm64'.

Changes:
- Add OS/arch detection in 'Determine version' step using uname -s/-m
- Map uname output to asset name format: x86_64→amd64, aarch64/arm64→arm64,
  linux→linux, darwin→darwin
- Emit 'os' and 'arch' as step outputs alongside 'version'
- Update cache key: review-bot-{os}-{arch}-{version}
- Update Install step: BINARY derived from step outputs
- Anchor checksum grep to exact filename (not substring match)
- Unsupported OS or arch exits with a clear error message

Supported platforms: linux-amd64, linux-arm64, darwin-amd64, darwin-arm64
(matches what the release workflow builds)
2026-05-14 05:45:20 +00:00
aweiker 5e351b85f0 Merge pull request 'feat(github): add safeguards against accidental AllowInsecureHTTP use (#96)' (#113) from review-bot-issue-96 into main
CI / test (push) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
Reviewed-on: #113
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
2026-05-13 20:21:42 +00:00
claw ab2a6c8aef Address review feedback on PR #113
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 22s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m33s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m47s
- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
  package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
  carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
  that no network call should occur (NIT #3)
2026-05-13 13:04:23 -07:00
claw 6b7f3f6924 fix: address NIT findings from sonnet review (#113)
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 35s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m7s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m32s
- Fix AllowInsecureHTTP doc comment: 'silently ignored' -> 'ignored' (it logs a warning)
- Remove unnecessary nil guard in redactURL (assigning nil to nil is a no-op)
- Add clarifying comment about acceptable double URL parse in doRequest
2026-05-13 12:48:02 -07:00
claw 4c032a3b53 fix: move AllowInsecureHTTPForTest to export_test.go, fix gofmt alignment
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m14s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m45s
2026-05-13 11:58:25 -07:00
claw 64c9d551ba fix: address review feedback — restore timer.Stop() and fix test spacing
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 24s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m14s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
  TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected

Resolves review comments from sonnet-review-bot on PR #113.
2026-05-13 11:44:28 -07:00
claw db7b7e66bf fix: use case-insensitive HTTP scheme check and redact userinfo
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m53s
Address review feedback on PR #113:

- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
  with url.Parse + strings.EqualFold for case-insensitive scheme
  comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.

- MINOR (security): Enhance redactURL to strip userinfo component
  (user:pass@host) in addition to query params, preventing credential
  leakage in error messages and logs.

- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
  it's a no-op and the comment was misleading.

- Add tests for uppercase/mixed-case HTTP scheme rejection and
  userinfo redaction.
2026-05-13 11:35:10 -07:00
claw 0232343126 feat(github): add safeguards against accidental AllowInsecureHTTP use in production
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 13s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m45s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m9s
Three-layer defense for the AllowInsecureHTTP client option:

1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
   env var. Without it, the option is silently ignored with a slog.Warn.

2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
   fires at client construction time so operators notice in production logs.

3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
   keeping test code clean (no t.Setenv needed in every test).

Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.

Closes #96
2026-05-13 11:24:07 -07:00
aweiker b26514714f Merge pull request 'feat(gitea): pass commit_id explicitly in PostReview (#107)' (#112) from review-bot-issue-107 into main
CI / test (push) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
Reviewed-on: #112
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 18:07:38 +00:00
10 changed files with 495 additions and 46 deletions
+42 -13
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
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
@@ -124,33 +124,62 @@ runs:
else
VERSION="${{ inputs.version }}"
fi
# Detect OS and architecture for platform-specific binary download
OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
case "$OS_RAW" in
linux) OS="linux" ;;
darwin) OS="darwin" ;;
*)
echo "Error: unsupported OS: $(uname -s)" >&2
exit 1
;;
esac
RAW_ARCH=$(uname -m)
case "$RAW_ARCH" in
x86_64) ARCH="amd64" ;;
aarch64 | arm64) ARCH="arm64" ;;
*)
echo "Error: unsupported architecture: $RAW_ARCH" >&2
exit 1
;;
esac
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "os=${OS}" >> "$GITHUB_OUTPUT"
echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary
id: cache
uses: actions/cache@v4
with:
path: ${{ runner.temp }}/review-bot
key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
key: review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-${{ steps.version.outputs.version }}
- name: Install review-bot
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-linux-amd64"
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
cd "${{ runner.temp }}"
EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
# sha256sum (GNU) is not available on macOS; use shasum -a 256 on darwin.
if [ "${{ steps.version.outputs.os }}" = "darwin" ]; then
ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')
else
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
fi
if [ -z "$EXPECTED" ]; then
echo "Error: no checksum found for ${BINARY}" >&2
@@ -164,12 +193,12 @@ runs:
fi
chmod +x "${{ runner.temp }}/review-bot"
echo "Installed review-bot ${VERSION} (checksum verified)"
echo "Installed review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }} ${VERSION} (checksum verified)"
- 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
## 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:
| 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)")
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() {
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 {
+79 -6
View File
@@ -8,7 +8,10 @@ import (
"errors"
"fmt"
"io"
"log/slog"
"net/http"
"net/url"
"os"
"strconv"
"strings"
"time"
@@ -93,10 +96,14 @@ type Client struct {
// Higher-level exported methods (GetPullRequest, etc.) will use it to
// construct request URLs; remove this field if those methods end up
// accepting full URLs instead.
baseURL string
token string
baseURL string
token string
httpClient *http.Client
// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
// When false, doRequest rejects URLs with an http:// scheme.
allowInsecureHTTP bool
// retryBackoff defines the delays between retry attempts for 429 responses.
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
// If nil, defaults to {1s, 2s}.
@@ -135,16 +142,53 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
return nil
}
// ClientOption configures optional behavior of a Client.
type ClientOption func(*clientConfig)
type clientConfig struct {
allowInsecureHTTP bool
insecureIsTestBypass bool
}
// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections.
// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1
// environment variable. Without the env var set, the option is ignored
// and a warning is logged.
//
// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.
func AllowInsecureHTTP() ClientOption {
return func(cfg *clientConfig) {
cfg.allowInsecureHTTP = true
}
}
// NewClient creates a new GitHub API client.
// If baseURL is empty, it defaults to https://api.github.com.
// For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
func NewClient(token, baseURL string) *Client {
func NewClient(token, baseURL string, opts ...ClientOption) *Client {
if baseURL == "" {
baseURL = defaultBaseURL
}
var cfg clientConfig
for _, opt := range opts {
opt(&cfg)
}
if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass {
if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" {
slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")
cfg.allowInsecureHTTP = false
} else {
slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
"env", "REVIEW_BOT_ALLOW_INSECURE=1")
}
}
return &Client{
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
allowInsecureHTTP: cfg.allowInsecureHTTP,
httpClient: &http.Client{
Timeout: 30 * time.Second,
CheckRedirect: defaultCheckRedirect,
@@ -215,10 +259,39 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
return 0, false
}
// redactURL redacts sensitive components from a URL for safe inclusion in error
// messages and log output. It removes userinfo (e.g., user:pass@) and replaces
// query parameters with a placeholder.
func redactURL(rawURL string) string {
u, err := url.Parse(rawURL)
if err != nil {
return "<unparseable URL>"
}
u.User = nil
if u.RawQuery != "" {
u.RawQuery = "<redacted>"
}
return u.String()
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present, supporting both integer
// seconds and HTTP-date formats (capped at maxRetryAfter).
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
// NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it
// again internally). Acceptable cost: URL parsing is cheap and threading the
// parsed *url.URL through would complicate the interface for negligible gain.
if !c.allowInsecureHTTP {
parsed, err := url.Parse(reqURL)
if err != nil {
return nil, fmt.Errorf("parse request URL: %w", err)
}
if strings.EqualFold(parsed.Scheme, "http") {
return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
}
}
var backoff []time.Duration
if c.retryBackoff != nil {
backoff = append([]time.Duration(nil), c.retryBackoff...)
@@ -237,7 +310,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay)
select {
case <-timer.C:
timer.Stop()
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
+154 -9
View File
@@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
}))
defer srv.Close()
c := NewClient("test-token", srv.URL)
c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
@@ -60,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -94,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.now = func() time.Time { return fixedNow }
// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
c.SetRetryBackoff([]time.Duration{0, 0})
@@ -130,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.now = func() time.Time { return fixedNow }
c.SetRetryBackoff([]time.Duration{0, 0})
@@ -157,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -187,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -208,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error, got nil")
@@ -230,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error, got nil")
@@ -260,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
ctx, cancel := context.WithCancel(context.Background())
@@ -511,3 +511,148 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
}
}
func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte("ok"))
}))
defer srv.Close()
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != "ok" {
t.Errorf("body = %q, want %q", body, "ok")
}
}
func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error for HTTP request without AllowInsecureHTTP")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
// Verify case-insensitive scheme check (RFC 3986).
c := NewClient("tok", "HTTP://127.0.0.1:1")
_, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test")
if err == nil {
t.Fatal("expected error for uppercase HTTP scheme")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
// Verify mixed case like "Http://" is also rejected.
c := NewClient("tok", "Http://127.0.0.1:1")
_, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test")
if err == nil {
t.Fatal("expected error for mixed-case HTTP scheme")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte("insecure-ok"))
}))
defer srv.Close()
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != "insecure-ok" {
t.Errorf("body = %q, want %q", body, "insecure-ok")
}
}
func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
// "true" is not "1" — strict check
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: env var 'true' is not '1'")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestRedactURL_WithQuery(t *testing.T) {
got := redactURL("http://localhost:1234/path?secret=token&foo=bar")
want := "http://localhost:1234/path?<redacted>"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_NoQuery(t *testing.T) {
got := redactURL("http://localhost:1234/path")
want := "http://localhost:1234/path"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_Userinfo(t *testing.T) {
got := redactURL("http://user:pass@localhost:1234/path")
want := "http://localhost:1234/path"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_UserinfoWithQuery(t *testing.T) {
got := redactURL("http://user:pass@localhost:1234/path?secret=token")
want := "http://localhost:1234/path?<redacted>"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
+13
View File
@@ -0,0 +1,13 @@
package github
// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
// This is intended exclusively for test code using httptest.Server.
//
// Defined in a _test.go file so it is only available to test binaries.
func AllowInsecureHTTPForTest() ClientOption {
return func(cfg *clientConfig) {
cfg.allowInsecureHTTP = true
cfg.insecureIsTestBypass = true
}
}
+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")