feat: sentinel-based review cleanup + system prompt file + security review
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 58s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m35s
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 58s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m35s
Sentinel-based cleanup: - Reviews embed <!-- review-bot:NAME --> in body (hidden HTML comment) - Cleanup matches by sentinel, not token identity - Each reviewer-name is a logical identity (sonnet, gpt, security) - Same token can run multiple review types without conflict - No extra API scopes needed System prompt file (--system-prompt-file / SYSTEM_PROMPT_FILE): - Loads a local file with additional review instructions - Appended to system base as "Additional Review Instructions" - Enables specialized reviews (security, performance, etc.) - Partially addresses #5 Security review: - SECURITY_REVIEW.md prompt focused on vulnerabilities - 3rd CI matrix entry using same token, different prompt - Focus: injection, auth, secrets, input validation, crypto, races CI changes: - REVIEWER_NAME passed from matrix.name - SYSTEM_PROMPT_FILE passed from matrix (empty for standard reviews) - 3 reviewers: sonnet (general), gpt (general), security (focused)
This commit is contained in:
@@ -70,6 +70,10 @@ inputs:
|
|||||||
description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)'
|
description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)'
|
||||||
required: false
|
required: false
|
||||||
default: 'true'
|
default: 'true'
|
||||||
|
system-prompt-file:
|
||||||
|
description: 'Local file with additional system prompt instructions (e.g. security review focus)'
|
||||||
|
required: false
|
||||||
|
default: ''
|
||||||
|
|
||||||
runs:
|
runs:
|
||||||
using: 'composite'
|
using: 'composite'
|
||||||
@@ -150,6 +154,7 @@ runs:
|
|||||||
LLM_TIMEOUT: ${{ inputs.timeout }}
|
LLM_TIMEOUT: ${{ inputs.timeout }}
|
||||||
LLM_PROVIDER: ${{ inputs.llm-provider }}
|
LLM_PROVIDER: ${{ inputs.llm-provider }}
|
||||||
UPDATE_EXISTING: ${{ inputs.update-existing }}
|
UPDATE_EXISTING: ${{ inputs.update-existing }}
|
||||||
|
SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
|
||||||
run: |
|
run: |
|
||||||
ARGS=""
|
ARGS=""
|
||||||
if [ "${{ inputs.dry-run }}" = "true" ]; then
|
if [ "${{ inputs.dry-run }}" = "true" ]; then
|
||||||
|
|||||||
@@ -32,6 +32,10 @@ jobs:
|
|||||||
- name: gpt
|
- name: gpt
|
||||||
token_secret: GPT_REVIEW_TOKEN
|
token_secret: GPT_REVIEW_TOKEN
|
||||||
model: gpt-4.1
|
model: gpt-4.1
|
||||||
|
- name: security
|
||||||
|
token_secret: SONNET_REVIEW_TOKEN
|
||||||
|
model: gpt-5
|
||||||
|
system_prompt_file: SECURITY_REVIEW.md
|
||||||
steps:
|
steps:
|
||||||
- uses: actions/checkout@v4
|
- uses: actions/checkout@v4
|
||||||
- uses: actions/setup-go@v5
|
- uses: actions/setup-go@v5
|
||||||
@@ -44,6 +48,7 @@ jobs:
|
|||||||
GITEA_REPO: ${{ github.repository }}
|
GITEA_REPO: ${{ github.repository }}
|
||||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||||
|
REVIEWER_NAME: ${{ matrix.name }}
|
||||||
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
|
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
|
||||||
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
|
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
|
||||||
LLM_MODEL: ${{ matrix.model }}
|
LLM_MODEL: ${{ matrix.model }}
|
||||||
@@ -51,4 +56,5 @@ jobs:
|
|||||||
PATTERNS_REPO: "rodin/go-patterns"
|
PATTERNS_REPO: "rodin/go-patterns"
|
||||||
PATTERNS_FILES: "README.md,patterns/"
|
PATTERNS_FILES: "README.md,patterns/"
|
||||||
LLM_TIMEOUT: "600"
|
LLM_TIMEOUT: "600"
|
||||||
|
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
|
||||||
run: ./review-bot
|
run: ./review-bot
|
||||||
|
|||||||
@@ -0,0 +1,18 @@
|
|||||||
|
You are performing a security-focused code review. Your primary concern is identifying vulnerabilities, not general code quality.
|
||||||
|
|
||||||
|
Focus areas:
|
||||||
|
- **Injection attacks**: SQL injection, command injection, path traversal, template injection
|
||||||
|
- **Authentication/Authorization**: Missing auth checks, privilege escalation, IDOR
|
||||||
|
- **Secrets exposure**: Hardcoded credentials, API keys in code, tokens in logs
|
||||||
|
- **Input validation**: Untrusted input used without sanitization, unsafe deserialization
|
||||||
|
- **Cryptography**: Weak algorithms, predictable randomness, improper key management
|
||||||
|
- **Error handling**: Information leakage in error messages, stack traces exposed
|
||||||
|
- **Dependencies**: Known vulnerable patterns, unsafe use of external libraries
|
||||||
|
- **Race conditions**: TOCTOU bugs, unsynchronized shared state
|
||||||
|
- **Resource exhaustion**: Unbounded allocations, missing timeouts, denial-of-service vectors
|
||||||
|
|
||||||
|
Rules for this review:
|
||||||
|
- Only report findings with actual security implications. Ignore style, naming, and general code quality.
|
||||||
|
- Severity mapping: MAJOR = exploitable vulnerability or data exposure. MINOR = defense-in-depth improvement or hardening opportunity. NIT = theoretical concern with low practical risk.
|
||||||
|
- If the code has no security-relevant changes, APPROVE with an empty findings list.
|
||||||
|
- Do not duplicate findings that a standard code review would catch (logic bugs, missing error checks) unless they have a security dimension.
|
||||||
+21
-4
@@ -30,6 +30,7 @@ func main() {
|
|||||||
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
|
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
|
||||||
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
|
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
|
||||||
conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
|
conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
|
||||||
|
systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions")
|
||||||
patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
|
patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
|
||||||
patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
|
patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
|
||||||
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
|
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
|
||||||
@@ -150,9 +151,24 @@ func main() {
|
|||||||
log.Printf("Loaded patterns from %s (%d bytes)", *patternsRepo, len(patterns))
|
log.Printf("Loaded patterns from %s (%d bytes)", *patternsRepo, len(patterns))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Step 6b: Load additional system prompt if specified
|
||||||
|
additionalPrompt := ""
|
||||||
|
if *systemPromptFile != "" {
|
||||||
|
data, err := os.ReadFile(*systemPromptFile)
|
||||||
|
if err != nil {
|
||||||
|
log.Fatalf("Failed to read system prompt file %q: %v", *systemPromptFile, err)
|
||||||
|
}
|
||||||
|
additionalPrompt = string(data)
|
||||||
|
log.Printf("Loaded system prompt file: %s (%d bytes)", *systemPromptFile, len(additionalPrompt))
|
||||||
|
}
|
||||||
|
|
||||||
// Step 7: Budget-aware prompt assembly
|
// Step 7: Budget-aware prompt assembly
|
||||||
|
systemBase := review.BuildSystemBase()
|
||||||
|
if additionalPrompt != "" {
|
||||||
|
systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
|
||||||
|
}
|
||||||
sections := budget.Sections{
|
sections := budget.Sections{
|
||||||
SystemBase: review.BuildSystemBase(),
|
SystemBase: systemBase,
|
||||||
Patterns: patterns,
|
Patterns: patterns,
|
||||||
Conventions: conventions,
|
Conventions: conventions,
|
||||||
FileContext: fileContext,
|
FileContext: fileContext,
|
||||||
@@ -203,14 +219,15 @@ func main() {
|
|||||||
}
|
}
|
||||||
log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login)
|
log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login)
|
||||||
|
|
||||||
// Delete stale reviews from this bot if update-existing is enabled
|
// Delete stale reviews from this bot using sentinel matching
|
||||||
if *updateExisting && posted.User.Login != "" {
|
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
|
||||||
|
if *updateExisting && *reviewerName != "" {
|
||||||
reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
log.Printf("Warning: could not list existing reviews: %v", err)
|
log.Printf("Warning: could not list existing reviews: %v", err)
|
||||||
} else {
|
} else {
|
||||||
for _, r := range reviews {
|
for _, r := range reviews {
|
||||||
if r.User.Login == posted.User.Login && r.ID != posted.ID {
|
if r.ID != posted.ID && strings.Contains(r.Body, sentinel) {
|
||||||
if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil {
|
if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil {
|
||||||
log.Printf("Warning: could not delete old review %d: %v", r.ID, err)
|
log.Printf("Warning: could not delete old review %d: %v", r.ID, err)
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -279,6 +279,7 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
|
|||||||
// Review represents a pull request review from the Gitea API.
|
// Review represents a pull request review from the Gitea API.
|
||||||
type Review struct {
|
type Review struct {
|
||||||
ID int64 `json:"id"`
|
ID int64 `json:"id"`
|
||||||
|
Body string `json:"body"`
|
||||||
User struct {
|
User struct {
|
||||||
Login string `json:"login"`
|
Login string `json:"login"`
|
||||||
} `json:"user"`
|
} `json:"user"`
|
||||||
|
|||||||
@@ -30,6 +30,8 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
|
|||||||
|
|
||||||
if reviewerName != "" {
|
if reviewerName != "" {
|
||||||
sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
|
sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
|
||||||
|
// Hidden sentinel for identifying this bot's reviews during cleanup
|
||||||
|
sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
|
||||||
}
|
}
|
||||||
|
|
||||||
return sb.String()
|
return sb.String()
|
||||||
|
|||||||
@@ -116,3 +116,21 @@ func TestGiteaEvent(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
||||||
|
result := &ReviewResult{
|
||||||
|
Verdict: "APPROVE",
|
||||||
|
Summary: "All good.",
|
||||||
|
Recommendation: "Merge it.",
|
||||||
|
}
|
||||||
|
output := FormatMarkdown(result, "security")
|
||||||
|
if !strings.Contains(output, "<!-- review-bot:security -->") {
|
||||||
|
t.Error("expected sentinel comment in output")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Empty reviewer name should NOT have sentinel
|
||||||
|
output2 := FormatMarkdown(result, "")
|
||||||
|
if strings.Contains(output2, "<!-- review-bot") {
|
||||||
|
t.Error("should not contain sentinel when reviewer name is empty")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user