CI: gate heavy reviews on self-review (Doc consistency); comment-trigger; disable TTL heavy reviews #159
@@ -5,11 +5,19 @@ on:
|
||||
branches: [main]
|
||||
pull_request:
|
||||
types: [opened, synchronize]
|
||||
issue_comment:
|
||||
types: [created, edited]
|
||||
|
||||
env:
|
||||
SELF_REVIEW_TTL_MIN: '45'
|
||||
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-24.04
|
||||
|
|
||||
if: github.event_name == 'pull_request'
|
||||
gpt-review-bot
commented
[MINOR] The test job is restricted to run only on pull_request events (if: github.event_name == 'pull_request'), so tests no longer run on push to main despite the workflow being triggered for pushes. If this was not intended, adjust the condition (e.g., exclude only issue_comment) to keep tests on push events. **[MINOR]** The test job is restricted to run only on pull_request events (if: github.event_name == 'pull_request'), so tests no longer run on push to main despite the workflow being triggered for pushes. If this was not intended, adjust the condition (e.g., exclude only issue_comment) to keep tests on push events.
|
||||
steps:
|
||||
|
gpt-review-bot
commented
[NIT] The test job installs jq even though it is unused in that job (jq is only needed in review-gate and review). Removing this step will speed up runs slightly. **[NIT]** The test job installs jq even though it is unused in that job (jq is only needed in review-gate and review). Removing this step will speed up runs slightly.
|
||||
- name: Install jq
|
||||
|
gpt-review-bot
commented
[MINOR] The test job installs jq but does not use it in any subsequent step, adding unnecessary time to test runs. Remove if not needed. **[MINOR]** The test job installs jq but does not use it in any subsequent step, adding unnecessary time to test runs. Remove if not needed.
|
||||
run: sudo apt-get update && sudo apt-get install -y jq
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-go@v5
|
||||
with:
|
||||
@@ -18,14 +26,58 @@ jobs:
|
||||
- run: go vet ./...
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
|
||||
[MINOR] issue_comment events will trigger the review-gate job for any PR comment, allowing an attacker to spam comments to repeatedly invoke the job and consume runner resources (DoS). Although heavy reviewers won't run without a valid self-review, the gate still performs package installs and API calls on each comment. **[MINOR]** issue_comment events will trigger the review-gate job for any PR comment, allowing an attacker to spam comments to repeatedly invoke the job and consume runner resources (DoS). Although heavy reviewers won't run without a valid self-review, the gate still performs package installs and API calls on each comment.
|
||||
# Self-review using native SAP AI Core provider
|
||||
# Models must match SAP AI Core deployments
|
||||
# Available models: gpt-5, anthropic--claude-4.6-sonnet, anthropic--claude-4.6-opus
|
||||
# Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Core
|
||||
review-gate:
|
||||
runs-on: ubuntu-24.04
|
||||
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `test` job now installs `jq` but never uses it. `jq` is only needed in the `review-gate` and `review` jobs. This adds ~5s to every test run for no reason.
|
||||
if: github.event_name == 'pull_request' || (github.event_name == 'issue_comment' && github.event.issue.pull_request)
|
||||
|
security-review-bot marked this conversation as resolved
[MINOR] The review-gate job is triggered on **[MINOR]** The review-gate job is triggered on `issue_comment` events for PRs, which is intended. For defense-in-depth, consider explicitly setting `permissions:` in the workflow to least-privilege (e.g., read-only) to reduce blast radius for the default token.
gpt-review-bot
commented
[MINOR] Condition uses (github.event.issue.pull_request) directly. For clarity and future-proofing, prefer an explicit null check (e.g., github.event.issue.pull_request != null) to avoid truthiness ambiguity. **[MINOR]** Condition uses (github.event.issue.pull_request) directly. For clarity and future-proofing, prefer an explicit null check (e.g., github.event.issue.pull_request != null) to avoid truthiness ambiguity.
|
||||
outputs:
|
||||
allow_review: ${{ steps.gate.outputs.allow_review }}
|
||||
reason: ${{ steps.gate.outputs.reason }}
|
||||
steps:
|
||||
- name: Install jq
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `REPO=${{ github.repository }}` and `API=...` lines use unquoted expression substitution without `${}` shell quoting. If the repository name ever contains spaces (unlikely but possible), this would break. Prefer `REPO="${{ github.repository }}"` style consistently.
|
||||
run: sudo apt-get update && sudo apt-get install -y jq
|
||||
- name: Check self-review gate
|
||||
id: gate
|
||||
env:
|
||||
GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}
|
||||
run: |
|
||||
set -e
|
||||
|
sonnet-review-bot
commented
[MINOR] The AGE_MIN variable is computed and ALLOW=true/REASON=ttl is set when TTL is exceeded, but **[MINOR]** The AGE_MIN variable is computed and ALLOW=true/REASON=ttl is set when TTL is exceeded, but `allow_review=true` with `reason=ttl` does NOT trigger the review job (the review job gates on `reason == 'self-review'`). This is correct per the stated intent, but the `allow_review` output is never actually consumed by any job — only `reason` is checked. The `allow_review` output is dead output and may cause future confusion. Consider removing it or documenting its lack of use.
[MINOR] The review-gate script uses **[MINOR]** The review-gate script uses `set -e` but not `-u` or `-o pipefail`. Adding `set -euo pipefail` would harden error handling and ensure failures in pipelines or unset variables don’t lead to unintended behavior (fail closed).
gpt-review-bot
commented
[MINOR] Shell robustness: consider using 'set -euo pipefail' and 'curl -fsS' to fail fast on unset vars and HTTP errors. Currently 'set -e' won't catch all error modes, and curl without -f treats HTTP 4xx/5xx as success. **[MINOR]** Shell robustness: consider using 'set -euo pipefail' and 'curl -fsS' to fail fast on unset vars and HTTP errors. Currently 'set -e' won't catch all error modes, and curl without -f treats HTTP 4xx/5xx as success.
|
||||
REPO=${{ github.repository }}
|
||||
API="${{ github.server_url }}/api/v1"
|
||||
if [ "${GITHUB_EVENT_NAME}" = "issue_comment" ]; then
|
||||
PR=${{ github.event.issue.number }}
|
||||
|
gpt-review-bot
commented
[MINOR] The review-gate condition **[MINOR]** The review-gate condition `(github.event_name == 'issue_comment' && github.event.issue.pull_request)` assumes object truthiness. For robustness, explicitly check existence (e.g., `github.event.issue.pull_request != null`) to avoid edge-case evaluation issues.
|
||||
else
|
||||
PR=${{ github.event.pull_request.number }}
|
||||
fi
|
||||
# Get head SHA from PR JSON (works for both events)
|
||||
|
sonnet-review-bot
commented
[MINOR] Shell variables from GitHub expressions are interpolated directly without quoting: **[MINOR]** Shell variables from GitHub expressions are interpolated directly without quoting: `REPO=${{ github.repository }}`. If the repository name contained spaces or special characters this would break. Prefer `REPO='${{ github.repository }}'` or export with quotes for robustness.
|
||||
PR_JSON=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/pulls/$PR")
|
||||
|
security-review-bot marked this conversation as resolved
[MINOR] Curl invocations lack explicit failure and timeout settings. Consider adding **[MINOR]** Curl invocations lack explicit failure and timeout settings. Consider adding `--fail` (or `--fail-with-body`), `--max-time`, and/or `--retry` to prevent hanging jobs and reduce DOS potential against CI if the API stalls.
|
||||
SHA=$(echo "$PR_JSON" | jq -r .head.sha)
|
||||
UPDATED_AT=$(echo "$PR_JSON" | jq -r .updated_at)
|
||||
NOW=$(date -u +%s)
|
||||
|
sonnet-review-bot
commented
[MINOR] The jq regex **[MINOR]** The jq regex `(?im)^###\s+Doc consistency\b` is passed through multiple layers of shell quoting and JSON encoding before reaching jq. The `\b` word-boundary assertion and the multiline mode should work in jq's ONIG engine, but this is non-obvious and fragile. A simpler `test("(?m)^### Doc consistency")` without the `\b` would be more readable and equally correct for this use case.
|
||||
PR_TS=$(date -u -d "$UPDATED_AT" +%s)
|
||||
AGE_MIN=$(( (NOW - PR_TS) / 60 ))
|
||||
TTL_MIN=${SELF_REVIEW_TTL_MIN}
|
||||
|
||||
COMMENTS=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/issues/$PR/comments?limit=200")
|
||||
|
security-review-bot marked this conversation as resolved
[MINOR] Same as above: the comments API request should include curl timeouts and **[MINOR]** Same as above: the comments API request should include curl timeouts and `--fail` to avoid indefinite hangs and fail closed on HTTP errors.
gpt-review-bot
commented
[NIT] Comments API call limits results to 200. Very large PRs with more than 200 comments could miss earlier valid self-reviews. Consider pagination or filtering by most recent comments if this is a concern. **[NIT]** Comments API call limits results to 200. Very large PRs with more than 200 comments could miss earlier valid self-reviews. Consider pagination or filtering by most recent comments if this is a concern.
|
||||
HAS_SR=$(echo "$COMMENTS" | jq -r --arg sha "$SHA" '[.[] | select(.user.login=="rodin") | select(.body|contains("Self-review against "+$sha)) | select(.body|test("(?im)^###\\s+Doc consistency\\b"))] | length')
|
||||
|
gpt-review-bot
commented
[MAJOR] Self-review gate only considers comments from user.login == "rodin". This contradicts the description that "Self‑review comments" should trigger reviews and will ignore valid self-reviews authored by PR authors, preventing heavy reviews from starting (TTL does not trigger reviewers). Remove or broaden this filter (e.g., allow the PR author and/or a configurable allowlist) to match intent. **[MAJOR]** Self-review gate only considers comments from user.login == "rodin". This contradicts the description that "Self‑review comments" should trigger reviews and will ignore valid self-reviews authored by PR authors, preventing heavy reviews from starting (TTL does not trigger reviewers). Remove or broaden this filter (e.g., allow the PR author and/or a configurable allowlist) to match intent.
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] The review job **[MINOR]** The review job `if:` condition uses string comparison `needs.review-gate.outputs.reason == 'self-review'` combined with `&&` against event_name checks. In Gitea Actions, if `review-gate` is skipped (e.g. for push events), `needs.review-gate.outputs.reason` will be empty and the comparison will be false — meaning review is silently skipped for push-to-main without any indication. This is probably intentional but worth a comment.
|
||||
if [ "$HAS_SR" -gt 0 ]; then
|
||||
ALLOW=true
|
||||
REASON=self-review
|
||||
elif [ "$AGE_MIN" -ge "$TTL_MIN" ]; then
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `date -u -d "$UPDATED_AT" +%s` uses GNU date syntax. This is fine on ubuntu-24.04, but the `-d` flag for parsing a date string is a GNU extension. Worth a comment so future maintainers know this is intentionally Linux-only.
|
||||
ALLOW=true
|
||||
REASON=ttl
|
||||
else
|
||||
ALLOW=false
|
||||
REASON=missing
|
||||
fi
|
||||
|
||||
echo "allow_review=$ALLOW" >> $GITHUB_OUTPUT
|
||||
|
sonnet-review-bot
commented
[MINOR] The TTL is calculated using **[MINOR]** The TTL is calculated using `PR_JSON.updated_at`, which changes on every push/comment. A PR with a new comment 5 minutes ago will never pass the TTL gate regardless of how old the PR is. If the intent is 'PR was opened and nobody reviewed it for 45 minutes', `created_at` would be more appropriate. If the intent is 'last activity was 45 minutes ago', the current behavior is correct but differs from the PR description which says 'TTL keeps the gate green'.
|
||||
echo "reason=$REASON" >> $GITHUB_OUTPUT
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `allow_review` output is set but never consumed — `review.if` only checks `reason`, not `allow_review`. The output could be removed, or the `review.if` condition could be simplified to just `needs.review-gate.outputs.reason == 'self-review'` (which it already is). The unused output is harmless but misleading.
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
if: github.event_name == 'pull_request'
|
||||
needs: test
|
||||
if: needs.review-gate.outputs.reason == 'self-review' && (github.event_name == 'pull_request' || github.event_name == 'issue_comment')
|
||||
needs: [review-gate]
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `review` job dropped its dependency on `test` (changed from `needs: test` to `needs: [review-gate]`). This means the heavy review can now run even if the Go build/tests are failing. The previous design required passing tests before reviews ran. This may be intentional but is worth confirming.
|
||||
@@ -39,19 +91,28 @@ jobs:
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
model: gpt-5
|
||||
patterns_repo: rodin/security-patterns
|
||||
patterns_files: "."
|
||||
patterns_files: '.'
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
steps:
|
||||
- name: Install jq
|
||||
run: sudo apt-get update && sudo apt-get install -y jq
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-go@v5
|
||||
with:
|
||||
go-version: '1.26'
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
- name: Set PR_NUMBER for event type
|
||||
run: |
|
||||
if [ "${GITHUB_EVENT_NAME}" = "issue_comment" ]; then
|
||||
echo "PR_NUMBER=${{ github.event.issue.number }}" >> $GITHUB_ENV
|
||||
else
|
||||
echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
|
||||
fi
|
||||
- name: Run ${{ matrix.name }} review
|
||||
env:
|
||||
VCS_URL: ${{ github.server_url }}
|
||||
GITEA_REPO: ${{ github.repository }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
PR_NUMBER: ${{ env.PR_NUMBER }}
|
||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||
REVIEWER_NAME: ${{ matrix.name }}
|
||||
LLM_PROVIDER: aicore
|
||||
@@ -61,9 +122,9 @@ jobs:
|
||||
AICORE_AUTH_URL: ${{ secrets.AICORE_AUTH_URL }}
|
||||
AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
|
||||
AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
|
||||
CONVENTIONS_FILE: "CONVENTIONS.md"
|
||||
CONVENTIONS_FILE: 'CONVENTIONS.md'
|
||||
PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
|
||||
PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
|
||||
LLM_TIMEOUT: "600"
|
||||
LLM_TIMEOUT: '600'
|
||||
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
|
||||
run: ./review-bot
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
name: Workflow Lint
|
||||
|
||||
on:
|
||||
push:
|
||||
branches: [main]
|
||||
pull_request:
|
||||
types: [opened, synchronize]
|
||||
|
||||
jobs:
|
||||
workflow-sanity:
|
||||
runs-on: ubuntu-24.04
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- name: Sanity check ci.yml triggers and gates
|
||||
run: |
|
||||
set -euo pipefail
|
||||
python3 - <<'PY'
|
||||
import sys, yaml, re
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `import sys, yaml, re` — `sys` and `re` are imported but `re` is unused (the regex is done via jq, not Python) and `sys` is unused too. This is harmless but slightly messy.
sonnet-review-bot
commented
[NIT] **[NIT]** `import sys, yaml, re` — `sys` and `re` are imported but never used in the script. Harmless, but slightly misleading.
gpt-review-bot
commented
[NIT] The Python script imports 're' but does not use it. Remove the unused import to reduce lint noise. **[NIT]** The Python script imports 're' but does not use it. Remove the unused import to reduce lint noise.
|
||||
from pathlib import Path
|
||||
p = Path('.gitea/workflows/ci.yml')
|
||||
|
gpt-review-bot
commented
[MINOR] The lint step relies on **[MINOR]** The lint step relies on `import yaml` (PyYAML) but does not install it. On many runners PyYAML is not preinstalled, risking sporadic failures. Add an installation step (e.g., `sudo apt-get install -y python3-yaml` or `pip install pyyaml`) or avoid the dependency.
|
||||
w = yaml.safe_load(p.read_text())
|
||||
|
gpt-review-bot
commented
[MAJOR] The lint step imports PyYAML (import yaml) but the job does not install it. Many runners do not have PyYAML preinstalled, so this job will fail with ModuleNotFoundError. Add an installation step (e.g., apt-get install -y python3-yaml or pip install pyyaml) before running the script. **[MAJOR]** The lint step imports PyYAML (import yaml) but the job does not install it. Many runners do not have PyYAML preinstalled, so this job will fail with ModuleNotFoundError. Add an installation step (e.g., apt-get install -y python3-yaml or pip install pyyaml) before running the script.
|
||||
# 1) Top-level 'on' must exist and include pull_request + issue_comment
|
||||
on = w.get('on')
|
||||
assert isinstance(on, dict), "ci.yml: top-level 'on' must be a mapping"
|
||||
assert 'pull_request' in on, "ci.yml: missing on.pull_request"
|
||||
assert 'issue_comment' in on, "ci.yml: missing on.issue_comment (self-review trigger)"
|
||||
pr_types = on['pull_request'].get('types', []) if isinstance(on['pull_request'], dict) else []
|
||||
ic_types = on['issue_comment'].get('types', []) if isinstance(on['issue_comment'], dict) else []
|
||||
for t in ['opened','synchronize']:
|
||||
assert t in pr_types, f"ci.yml: pull_request.types must include '{t}'"
|
||||
for t in ['created','edited']:
|
||||
assert t in ic_types, f"ci.yml: issue_comment.types must include '{t}'"
|
||||
# 2) review-gate must run on both PR and issue_comment (if condition string)
|
||||
rg_if = w['jobs']['review-gate'].get('if','')
|
||||
assert 'github.event_name == ' in rg_if and 'issue_comment' in rg_if and 'pull_request' in rg_if, \
|
||||
"ci.yml: review-gate.if must include both pull_request and issue_comment"
|
||||
# 3) review job must require self-review reason
|
||||
rev_if = w['jobs']['review'].get('if','')
|
||||
assert "needs.review-gate.outputs.reason == 'self-review'" in rev_if, \
|
||||
"ci.yml: review.if must require reason=='self-review'"
|
||||
print('OK: ci.yml triggers and gates look sane')
|
||||
PY
|
||||
[MAJOR] The test job is restricted to only pull_request events via
if: github.event_name == 'pull_request', so tests no longer run on push to main even though the workflow still triggers on push. This likely degrades CI coverage and branch protection; prefer skipping onlyissue_commentruns or include push explicitly (e.g.,if: github.event_name != 'issue_comment'orif: github.event_name == 'pull_request' || github.event_name == 'push').