CI: gate heavy reviews on self-review (Doc consistency); comment-trigger; disable TTL heavy reviews #159
Open
rodin
wants to merge 8 commits from
ci-selfreview-gate into main
pull from: ci-selfreview-gate
merge into: rodin:main
rodin:main
rodin:ci/cleanup
rodin:issue-150
rodin:issue-157
rodin:issue-141
rodin:issue-154
rodin:review-bot-dev-loop
rodin:issue-143
rodin:issue-146
rodin:pr-153
rodin:review-bot-issue-130-work
rodin:issue-148
rodin:issue-139
rodin:issue-137
rodin:review-bot-fixes
rodin:review-bot-issue-133
rodin:review-bot-issue-130
rodin:issue-130
rodin:github-support
rodin:issue-123-work
rodin:issue-123
rodin:review-bot-issue-120
rodin:fix/125-readme-cli-example
rodin:issue-125
rodin:issue-124
rodin:issue-120
rodin:feature/github-support
rodin:review-bot-issue-116
rodin:review-bot-issue-115
rodin:review-bot-issue-114
rodin:review-bot-issue-96
rodin:review-bot-issue-107
rodin:review-bot-issue-82
rodin:review-bot-issue-95
rodin:review-bot-issue-92
rodin:review-bot-issue-94
rodin:review-bot-issue-81
rodin:review-bot-issue-91
rodin:review-bot-issue-97
rodin:issue-80-c-file-reader
rodin:issue-80-b-pr-reader
rodin:issue-80-a-client
rodin:review-bot-issue-80
rodin:review-bot-issue-87
rodin:review-bot-issue-79
rodin:review-bot-issue-84
rodin:review-bot-issue-78
rodin:issue-73
rodin:issue-70
rodin:issue-68
rodin:issue-66
rodin:issue-64
rodin:issue-60-remote-personas
rodin:issue-60
rodin:issue-57
rodin:allow-deps
rodin:feat/aicore-provider-v2
rodin:issue-51
rodin:ci/pr-ready-gate
rodin:fix/stale-commit-check
rodin:feat/aicore-provider
rodin:fix/response-body-truncation
rodin:fix/json-repair
rodin:fix/sonnet-reviewer
rodin:fix/consistent-path-escape
rodin:feat/inline-review-comments
rodin:feat/6-update-existing-review
rodin:fix/19-context-overflow
rodin:feat/18-anthropic-api
rodin:fix/url-escaping-and-shadow
rodin:fix/quick-wins
rodin:fix/context-and-encapsulation
rodin:docs/code-review-report
rodin:ci/release-workflow
Dismiss Review
Are you sure you want to dismiss this review?
No Label
self-reviewed
Milestone
No items
No Milestone
Projects
Clear projects
No project
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: rodin/review-bot#159
Reference in New Issue
Block a user
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.
Delete Branch "ci-selfreview-gate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Gate the heavy review job until a current self‑review exists with a “Doc consistency” section. TTL keeps the gate green but does NOT start heavy reviews. Self‑review comments now trigger the gate and, when valid, immediately start the heavy review.
Changes:
Rationale:
CI: gate review on self-review (Doc consistency) + TTL fallbackto CI: gate heavy reviews on self-review (Doc consistency); comment-trigger; disable TTL heavy reviewsFix Plan against
97b688f95f:What is broken
The
review-gateCI job fails at the "Check self-review gate" step with:The jq filter in
.gitea/workflows/ci.ymlcontains:Why it fails
The regex
(?im)^###\s+Doc consistency\bis passed as a JSON string literal to jq'stest()builtin. In JSON string context,\sand\bare invalid JSON escape sequences (JSON only supports\n,\r,\t,\\,\",\/, and\uXXXX). jq rejects the string at parse time.Exact change needed
In
.gitea/workflows/ci.yml, change the jq regex string from:to:
Double-escaping
\\sand\\bproduces the literal characters\sand\bafter JSON string parsing, which jq's regex engine then correctly interprets as whitespace and word-boundary respectively.Scope
.gitea/workflows/ci.ymltestjob passes; onlyreview-gateis brokenSelf-Review: PR #159
Self-review against
d1ce39bd7bPhase 1: Independent Findings
None — diff looks clean.
The single changed line:
Correctness:
\sand\bare invalid JSON escape sequences. jq parsestest()arguments as JSON strings, so both sequences cause a compile error at runtime. Double-escaping (\\s→\safter JSON parsing → jq PCRE whitespace class;\\b→\b→ jq PCRE word boundary) is the correct and only fix.Scope: One-line change in
.gitea/workflows/ci.yml. No Go code changed. No regressions possible.Edge cases: None. The regex intent is preserved exactly. The flag group
(?im)is unaffected.Phase 2: Prior Review Verification
\s/\bare invalid JSON escapes in jq test(); need double-escapingDoc consistency
No documentation changes needed — this is a CI YAML bug fix with no user-facing behavior change.
Assessment: ✅ Clean
Fix is correct, minimal, and complete. Implements the fix plan exactly. Ready for heavy review.
Self-Review: PR #159
Self-review against
951aa5d584Phase 1: Independent Findings
This HEAD adds one commit on top of the previously-reviewed
d1ce39bd:ci: add workflow-lint to sanity-check triggers and gates for ci.yml.The diff introduces two changed files:
.gitea/workflows/ci.yml(+71/-10) — Addsreview-gatejob, gates heavy reviews on self-review presence, addsissue_commenttrigger, guardstestjob to PR-only events, addsSELF_REVIEW_TTL_MINenv var, and installsjqwhere needed..gitea/workflows/workflow-lint.yml(+42/-0) — New workflow that validatesci.ymltriggers and gate conditions using Python/YAML.Correctness of the jq fix (core change from earlier commits, still present):
test("(?im)^###\\s+Doc consistency\\b")— double-backslash in YAML → single backslash in shell → valid PCRE\s/\bin jq. Fix is correct.review-gatelogic:.head.sha, matches against self-review comments byrodincontaining both"Self-review against <sha>"and### Doc consistency. Correct.AGE_MIN >= TTL_MIN): setsALLOW=truebutREASON=ttl. Thereviewjob only runs onreason=='self-review', so TTL never triggers reviewers. Matches stated intent.PR_NUMBERis set per event type via$GITHUB_ENVand consumed as${{ env.PR_NUMBER }}in the subsequent step — standard pattern, works correctly in GitHub Actions.reviewjob dependency change:needs: test(tests must pass before review runs)needs: [review-gate](tests no longer block review)workflow-lint.yml:review-gate.if, andreview.ifvia Python assertions.None — diff looks clean.
Phase 2: Prior Review Verification
\s/\bare invalid JSON escapes in jqtest(); need double-escaping\\s/\\bin YAML → correct PCRE escapes in jqd1ce39bd(Assessment: ✅ Clean)951aa5d; this review covers the new commitDoc consistency
No documentation changes needed — CI workflow and lint additions have no user-facing behavior change.
Assessment: ✅ Clean
All changes are correct and complete. The workflow-lint addition is a good defensive measure. The review-gate logic is sound. Ready for human review.
Nudge
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
951aa5d5)Sonnet Review
Summary
CI workflow changes are well-structured and correct. The gate logic is sound, the TTL-vs-self-review distinction is properly enforced, and the new workflow-lint sanity check provides good self-validation. No Go code changes are present, so language patterns are not applicable.
Findings
.gitea/workflows/ci.yml(?im)^###\s+Doc consistency\bis passed through multiple layers of shell quoting and JSON encoding before reaching jq. The\bword-boundary assertion and the multiline mode should work in jq's ONIG engine, but this is non-obvious and fragile. A simplertest("(?m)^### Doc consistency")without the\bwould be more readable and equally correct for this use case..gitea/workflows/ci.ymlallow_review=truewithreason=ttldoes NOT trigger the review job (the review job gates onreason == 'self-review'). This is correct per the stated intent, but theallow_reviewoutput is never actually consumed by any job — onlyreasonis checked. Theallow_reviewoutput is dead output and may cause future confusion. Consider removing it or documenting its lack of use..gitea/workflows/ci.ymlif:condition uses string comparisonneeds.review-gate.outputs.reason == 'self-review'combined with&&against event_name checks. In Gitea Actions, ifreview-gateis skipped (e.g. for push events),needs.review-gate.outputs.reasonwill 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..gitea/workflows/ci.ymlREPO=${{ github.repository }}andAPI=...lines use unquoted expression substitution without${}shell quoting. If the repository name ever contains spaces (unlikely but possible), this would break. PreferREPO="${{ github.repository }}"style consistently..gitea/workflows/workflow-lint.ymlimport sys, yaml, re—sysandreare imported butreis unused (the regex is done via jq, not Python) andsysis unused too. This is harmless but slightly messy.Recommendation
APPROVE — Approve with minor notes. The core logic is correct: self-review gates heavy reviews, TTL only prevents red gates without triggering reviewers, and issue_comment events are properly handled for both events. The workflow-lint is a nice self-validating addition. The findings are all minor or nit-level — none affect correctness of the gating logic. The dead
allow_reviewoutput (MINOR finding) is the most worth addressing for future maintainability, but it doesn't break anything today.Review by sonnet
Evaluated against
951aa5d5@@ -25,0 +33,4 @@allow_review: ${{ steps.gate.outputs.allow_review }}reason: ${{ steps.gate.outputs.reason }}steps:- name: Install jq[NIT] The
REPO=${{ github.repository }}andAPI=...lines use unquoted expression substitution without${}shell quoting. If the repository name ever contains spaces (unlikely but possible), this would break. PreferREPO="${{ github.repository }}"style consistently.@@ -25,0 +40,4 @@env:GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}run: |set -e[MINOR] The AGE_MIN variable is computed and ALLOW=true/REASON=ttl is set when TTL is exceeded, but
allow_review=truewithreason=ttldoes NOT trigger the review job (the review job gates onreason == 'self-review'). This is correct per the stated intent, but theallow_reviewoutput is never actually consumed by any job — onlyreasonis checked. Theallow_reviewoutput is dead output and may cause future confusion. Consider removing it or documenting its lack of use.@@ -25,0 +52,4 @@PR_JSON=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/pulls/$PR")SHA=$(echo "$PR_JSON" | jq -r .head.sha)UPDATED_AT=$(echo "$PR_JSON" | jq -r .updated_at)NOW=$(date -u +%s)[MINOR] The jq regex
(?im)^###\s+Doc consistency\bis passed through multiple layers of shell quoting and JSON encoding before reaching jq. The\bword-boundary assertion and the multiline mode should work in jq's ONIG engine, but this is non-obvious and fragile. A simplertest("(?m)^### Doc consistency")without the\bwould be more readable and equally correct for this use case.@@ -25,0 +59,4 @@COMMENTS=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/issues/$PR/comments?limit=200")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')[MINOR] The review job
if:condition uses string comparisonneeds.review-gate.outputs.reason == 'self-review'combined with&&against event_name checks. In Gitea Actions, ifreview-gateis skipped (e.g. for push events),needs.review-gate.outputs.reasonwill 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.@@ -0,0 +15,4 @@run: |set -euo pipefailpython3 - <<'PY'import sys, yaml, re[NIT]
import sys, yaml, re—sysandreare imported butreis unused (the regex is done via jq, not Python) andsysis unused too. This is harmless but slightly messy.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
951aa5d5)Gpt Review
Summary
Overall the gating logic and comment-triggered heavy review look solid and CI passed, but the change unintentionally disables running tests on push events to main. This weakens branch protection and contradicts the repository’s process conventions. Minor robustness issues exist in the workflow lint job.
Findings
.gitea/workflows/ci.ymlif: 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'or `if: github.event_name == 'pull_request'.gitea/workflows/workflow-lint.ymlimport 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-yamlorpip install pyyaml) or avoid the dependency..gitea/workflows/ci.yml.gitea/workflows/ci.yml(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.Recommendation
REQUEST_CHANGES — The PR largely achieves the intended gating behavior and comment-triggered execution, but please address the CI regression where tests no longer run on push to main. Update the test job condition to either exclude only issue_comment events or also include push events, for example:
github.event_name != 'issue_comment'github.event_name == 'pull_request' || github.event_name == 'push'This preserves fast comment-triggered runs while retaining test coverage on pushes to main.
Additionally, make the workflow-lint job robust by ensuring PyYAML is available (install
python3-yamlorpip install pyyaml) or replace the Python YAML parsing with a tooling approach that doesn’t require extra dependencies. Finally, consider removing the unnecessary jq installation from the test job to reduce runtime. With these adjustments, the workflows will be both robust and aligned with the repository’s testing process.Review by gpt
Evaluated against
951aa5d5@@ -8,4 +13,4 @@jobs:test:runs-on: ubuntu-24.04[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').@@ -10,3 +15,4 @@test:runs-on: ubuntu-24.04if: github.event_name == 'pull_request'steps:[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.
@@ -25,0 +44,4 @@REPO=${{ github.repository }}API="${{ github.server_url }}/api/v1"if [ "${GITHUB_EVENT_NAME}" = "issue_comment" ]; thenPR=${{ github.event.issue.number }}[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.@@ -0,0 +17,4 @@python3 - <<'PY'import sys, yaml, refrom pathlib import Pathp = Path('.gitea/workflows/ci.yml')[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-yamlorpip install pyyaml) or avoid the dependency.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
951aa5d5)Security Review
Summary
Workflow changes gate heavy reviews on a verified self-review and add a lint job to sanity-check triggers. No exploitable security issues are introduced; gating logic uses a token securely and avoids leaking secrets.
Findings
.gitea/workflows/ci.ymlset -ebut not-uor-o pipefail. Addingset -euo pipefailwould harden error handling and ensure failures in pipelines or unset variables don’t lead to unintended behavior (fail closed)..gitea/workflows/ci.yml--fail(or--fail-with-body),--max-time, and/or--retryto prevent hanging jobs and reduce DOS potential against CI if the API stalls..gitea/workflows/ci.yml--failto avoid indefinite hangs and fail closed on HTTP errors..gitea/workflows/ci.ymlissue_commentevents for PRs, which is intended. For defense-in-depth, consider explicitly settingpermissions:in the workflow to least-privilege (e.g., read-only) to reduce blast radius for the default token.Recommendation
APPROVE — Overall, the gating logic is implemented securely: it uses an authenticated API call with a repository token, avoids echoing secrets, filters comments by a specific bot account and content pattern, and ensures heavy reviews only run when the reason is explicitly 'self-review'. To harden further against availability risks and ensure fail-closed behavior, update the review-gate run step to use
set -euo pipefail, and add curl options like--fail(or--fail-with-body) and--max-time(and optionally--retry) on both API calls. Also consider adding explicitpermissions:to the workflow to follow least privilege for the CI token. With these minor defenses-in-depth, the change set looks sound.Review by security
Evaluated against
951aa5d5@@ -24,1 +29,3 @@# Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Corereview-gate:runs-on: ubuntu-24.04if: github.event_name == 'pull_request' || (github.event_name == 'issue_comment' && github.event.issue.pull_request)[MINOR] The review-gate job is triggered on
issue_commentevents for PRs, which is intended. For defense-in-depth, consider explicitly settingpermissions:in the workflow to least-privilege (e.g., read-only) to reduce blast radius for the default token.@@ -25,0 +40,4 @@env:GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}run: |set -e[MINOR] The review-gate script uses
set -ebut not-uor-o pipefail. Addingset -euo pipefailwould harden error handling and ensure failures in pipelines or unset variables don’t lead to unintended behavior (fail closed).@@ -25,0 +49,4 @@PR=${{ github.event.pull_request.number }}fi# Get head SHA from PR JSON (works for both events)PR_JSON=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/pulls/$PR")[MINOR] Curl invocations lack explicit failure and timeout settings. Consider adding
--fail(or--fail-with-body),--max-time, and/or--retryto prevent hanging jobs and reduce DOS potential against CI if the API stalls.@@ -25,0 +57,4 @@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")[MINOR] Same as above: the comments API request should include curl timeouts and
--failto avoid indefinite hangs and fail closed on HTTP errors.Sonnet Review
Summary
CI workflow change that gates heavy reviews on a self-review comment and adds a workflow linter. The logic is sound and the implementation is clean. A few minor shell robustness and logic issues worth noting, but nothing blocking.
Findings
.gitea/workflows/ci.ymlREPO=${{ github.repository }}. If the repository name contained spaces or special characters this would break. PreferREPO='${{ github.repository }}'or export with quotes for robustness..gitea/workflows/ci.ymldate -u -d "$UPDATED_AT" +%suses GNU date syntax. This is fine on ubuntu-24.04, but the-dflag for parsing a date string is a GNU extension. Worth a comment so future maintainers know this is intentionally Linux-only..gitea/workflows/ci.ymlPR_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_atwould 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'..gitea/workflows/ci.ymlallow_reviewoutput is set but never consumed —review.ifonly checksreason, notallow_review. The output could be removed, or thereview.ifcondition could be simplified to justneeds.review-gate.outputs.reason == 'self-review'(which it already is). The unused output is harmless but misleading..gitea/workflows/ci.ymltestjob now installsjqbut never uses it.jqis only needed in thereview-gateandreviewjobs. This adds ~5s to every test run for no reason..gitea/workflows/ci.ymlreviewjob dropped its dependency ontest(changed fromneeds: testtoneeds: [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..gitea/workflows/workflow-lint.ymlimport sys, yaml, re—sysandreare imported but never used in the script. Harmless, but slightly misleading.Recommendation
APPROVE — Approve with minor notes. The overall design is correct and well-structured: the gate logic is clear, the workflow linter is a good self-enforcing safeguard, and the issue_comment trigger is properly guarded with
github.event.issue.pull_request. The most actionable findings are: (1) removing the unusedjqinstall from thetestjob, (2) confirming whetherupdated_atorcreated_atis the right TTL anchor, and (3) confirming intentionality of removing theneeds: testdependency from thereviewjob. None of these are blocking.Review by sonnet
Evaluated against
951aa5d5@@ -23,2 +29,2 @@# 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 Corereview-gate:runs-on: ubuntu-24.04[MINOR] The
testjob now installsjqbut never uses it.jqis only needed in thereview-gateandreviewjobs. This adds ~5s to every test run for no reason.@@ -25,0 +48,4 @@elsePR=${{ github.event.pull_request.number }}fi# Get head SHA from PR JSON (works for both events)[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. PreferREPO='${{ github.repository }}'or export with quotes for robustness.@@ -25,0 +63,4 @@if [ "$HAS_SR" -gt 0 ]; thenALLOW=trueREASON=self-reviewelif [ "$AGE_MIN" -ge "$TTL_MIN" ]; then[MINOR]
date -u -d "$UPDATED_AT" +%suses GNU date syntax. This is fine on ubuntu-24.04, but the-dflag for parsing a date string is a GNU extension. Worth a comment so future maintainers know this is intentionally Linux-only.@@ -25,0 +71,4 @@REASON=missingfiecho "allow_review=$ALLOW" >> $GITHUB_OUTPUT[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_atwould 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'.@@ -25,0 +73,4 @@echo "allow_review=$ALLOW" >> $GITHUB_OUTPUTecho "reason=$REASON" >> $GITHUB_OUTPUT[MINOR] The
allow_reviewoutput is set but never consumed —review.ifonly checksreason, notallow_review. The output could be removed, or thereview.ifcondition could be simplified to justneeds.review-gate.outputs.reason == 'self-review'(which it already is). The unused output is harmless but misleading.@@ -29,3 +80,4 @@needs: [review-gate]strategy:matrix:include:[NIT] The
reviewjob dropped its dependency ontest(changed fromneeds: testtoneeds: [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.@@ -0,0 +15,4 @@run: |set -euo pipefailpython3 - <<'PY'import sys, yaml, re[NIT]
import sys, yaml, re—sysandreare imported but never used in the script. Harmless, but slightly misleading.Security Review
Summary
The workflow changes gate heavy reviews behind a bot-authored self-review and correctly avoid running reviewers on TTL alone. Secrets are not echoed, API calls are scoped to the Gitea server, and the comment trigger is constrained to PR comments, which limits abuse. CI has passed.
Findings
.gitea/workflows/ci.ymlRecommendation
APPROVE — Overall, the gating logic is sound and avoids exposing secrets or running expensive reviewers without a valid, bot-authored self-review. To harden against CI resource exhaustion via comment spam, consider: (1) further constraining the review-gate job to only run on issue_comment events when the actor is the expected bot (e.g., github.event.comment.user.login == 'rodin'), or (2) adding an early, cheap step that exits before installing packages when the comment author != 'rodin'. This keeps the fast-start behavior when the bot posts/edits the self-review while avoiding unnecessary work on unrelated comments. No other security issues were identified.
Review by security
Evaluated against
951aa5d5@@ -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.
Gpt Review
Summary
Overall the workflow changes are close to the stated goals, but there are two critical issues: the gate only accepts self-review comments authored by the 'rodin' account (likely excluding PR author self-reviews), and the new workflow-lint job depends on PyYAML without installing it, risking CI breakage on runners without it. There are also a few robustness and clarity improvements worth addressing.
Findings
.gitea/workflows/ci.yml.gitea/workflows/workflow-lint.yml.gitea/workflows/ci.yml.gitea/workflows/ci.yml.gitea/workflows/ci.yml.gitea/workflows/ci.yml.gitea/workflows/workflow-lint.yml.gitea/workflows/ci.ymlRecommendation
REQUEST_CHANGES — Please address the following before merging:
Fix the self-review author filter: remove the hardcoded 'select(.user.login=="rodin")' in the gate's jq filter or expand it to include the PR author and/or a configurable allowlist (e.g., allow comments by the PR author and the 'rodin' bot). This ensures valid self-review comments trigger heavy reviews as described.
Ensure the workflow-lint job installs PyYAML. Add a step such as 'sudo apt-get update && sudo apt-get install -y python3-yaml' or 'pip install pyyaml' before running the Python script, so the job is portable across runners.
Revisit the test job condition if you still want tests on push to main. If your goal is to avoid running tests on issue_comment events only, change the condition to 'if: github.event_name != 'issue_comment'' (or keep both 'pull_request' and 'push') so push-to-main continues to run tests.
Hardening and cleanup: update the gate script to use 'set -euo pipefail' and 'curl -fsS' (both API calls) to fail fast on errors; consider making the issue_comment PR check explicit with '!= null'; remove the unused jq installation from the test job; and drop the unused 're' import in the lint script. Optionally, document or handle pagination for comment retrieval if very large PRs are expected.
With these fixes, the workflows will align with the intended behavior (comment-triggered heavy reviews on valid self-reviews, TTL only keeping the gate green) and be more robust across environments.
Review by gpt
Evaluated against
951aa5d5@@ -9,3 +14,4 @@jobs:test:runs-on: ubuntu-24.04if: github.event_name == 'pull_request'[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.
@@ -11,2 +16,4 @@runs-on: ubuntu-24.04if: github.event_name == 'pull_request'steps:- name: Install jq[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.
@@ -24,1 +29,3 @@# Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Corereview-gate:runs-on: ubuntu-24.04if: github.event_name == 'pull_request' || (github.event_name == 'issue_comment' && github.event.issue.pull_request)[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.
@@ -25,0 +40,4 @@env:GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}run: |set -e[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.
@@ -25,0 +57,4 @@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")[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.
@@ -25,0 +58,4 @@TTL_MIN=${SELF_REVIEW_TTL_MIN}COMMENTS=$(curl -sS -H "Authorization: token $GITEA_TOKEN" "$API/repos/$REPO/issues/$PR/comments?limit=200")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')[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.
@@ -0,0 +15,4 @@run: |set -euo pipefailpython3 - <<'PY'import sys, yaml, re[NIT] The Python script imports 're' but does not use it. Remove the unused import to reduce lint noise.
@@ -0,0 +18,4 @@import sys, yaml, refrom pathlib import Pathp = Path('.gitea/workflows/ci.yml')w = yaml.safe_load(p.read_text())[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.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.