feat: configurable review rules and AI PR description summarize#7
Conversation
Add severity selection, block-check threshold, and ignore-file-types to the existing repository_rule setting, with prompt+filter severity enforcement, configurable merge-gate threshold, and pre-review file filtering.
Add summarize_pr_description toggle to repository_rule settings. When enabled, Reviewly rewrites PR description with AI summary on every review run, preserving original text below.
… prompts Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…hing Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…arize Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…ntrols Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
✅ Coverage Report
|
![reviewly-2026-06-08-ztpjjy[bot]](https://avatars.githubusercontent.com/u/34906923?s=60&v=4){kind=small}
| strings.Join(labels, ", ") + | ||
| ". Ignore findings outside this selected severity set." | ||
| } | ||
|
|
There was a problem hiding this comment.
Ignore matching is not OS-path-safe and can fail to ignore files on Windows paths
Using path.Clean and checking only for "/" assumes slash-separated paths. If filePath comes from Git or filesystem code with backslashes, base extraction and pattern routing can fail, causing configured ignore patterns to be bypassed and sensitive or excluded files to still be processed.
| func shouldIgnoreFile(filePath string, patterns []string) bool { | |
| cleanedPath := strings.TrimPrefix(path.Clean(strings.ReplaceAll(strings.TrimSpace(filePath), "\\", "/")), "./") | |
| if cleanedPath == "." || cleanedPath == "" { | |
| return false | |
| } | |
| base := path.Base(cleanedPath) | |
| for _, rawPattern := range patterns { | |
| pattern := strings.TrimSpace(strings.ReplaceAll(rawPattern, "\\", "/")) | |
| if pattern == "" { | |
| continue | |
| } | |
| target := cleanedPath | |
| if !strings.Contains(pattern, "/") { | |
| target = base | |
| if strings.HasPrefix(pattern, ".") { | |
| pattern = "*" + pattern | |
| } | |
| } | |
| if globMatch(pattern, target) { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
| return response, errors.New(fiber.StatusBadRequest, errors.ErrRepositoryRuleEmpty) | ||
| } | ||
|
|
||
| isEnabled := true |
There was a problem hiding this comment.
Empty repository rules can now be saved, breaking downstream rule processing
The previous guard rejecting blank/whitespace-only rule content was removed. This allows persisting enabled rules with empty content, which can lead to invalid configuration being treated as active and cause failures or bypasses in downstream evaluation that assumes a non-empty rule body.
| isEnabled := true | |
| if len(strings.TrimSpace(request.Content)) == 0 { | |
| logger.Errorf("empty repository rule content for repository %d", request.RepositoryID) | |
| return response, errors.New(fiber.StatusBadRequest, errors.ErrRepositoryRuleEmpty) | |
| } | |
| isEnabled := true |
| @@ -5,7 +5,6 @@ import ( | |||
| "encoding/json" | |||
There was a problem hiding this comment.
Removed import is still required once blank-content validation is restored
The diff drops the strings import even though safe validation of rule content requires trimming whitespace before accepting it. Without reinstating that import, the necessary fix will not compile.
| "encoding/json" | |
| import ( | |
| "context" | |
| "encoding/json" | |
| stderrors "errors" | |
| "strconv" | |
| "strings" | |
| "github.com/gofiber/fiber/v3" | |
| "github.com/miqdadyyy/reviewly/internal/pkg/repository" | |
| ) |
| await upsertMutation.mutateAsync({ | ||
| repository_id: selectedRepoId, | ||
| content, | ||
| is_enabled: isEnabled, |
There was a problem hiding this comment.
Unsaved ignored file type input is dropped when saving
If the user types a pattern and clicks Save without pressing Enter/comma or blurring the field first, the current input is never committed and is silently omitted from the persisted rule.
| is_enabled: isEnabled, | |
| const handleSave = async () => { | |
| if (selectedRepoId === null) return | |
| const pendingIgnoredFileTypes = ignoredFileTypeInput | |
| .split(",") | |
| .map((item) => item.trim()) | |
| .filter((item) => item.length > 0) | |
| const nextIgnoredFileTypes = Array.from( | |
| new Set([...ignoredFileTypes, ...pendingIgnoredFileTypes]) | |
| ) | |
| await upsertMutation.mutateAsync({ | |
| repository_id: selectedRepoId, | |
| content, | |
| is_enabled: isEnabled, | |
| severities, | |
| block_threshold: blockThreshold, | |
| ignored_file_types: nextIgnoredFileTypes, | |
| summarize_pr_description: summarizePRDescription, | |
| }) | |
| setIgnoredFileTypes(nextIgnoredFileTypes) | |
| setIgnoredFileTypeInput("") | |
| setSavedAt(Date.now()) | |
| } |
| is_enabled: isEnabled, | ||
| severities, | ||
| block_threshold: blockThreshold, | ||
| ignored_file_types: ignoredFileTypes, |
There was a problem hiding this comment.
Block threshold can be saved for severities that are not reviewed
The UI says only reviewed severities can block, but handleSave sends any selected blockThreshold even when that severity is unchecked. This can create an impossible or backend-invalid configuration such as reviewing only critical issues while blocking on medium.
| ignored_file_types: ignoredFileTypes, | |
| const handleSave = async () => { | |
| if (selectedRepoId === null) return | |
| const pendingIgnoredFileTypes = ignoredFileTypeInput | |
| .split(",") | |
| .map((item) => item.trim()) | |
| .filter((item) => item.length > 0) | |
| const nextIgnoredFileTypes = Array.from( | |
| new Set([...ignoredFileTypes, ...pendingIgnoredFileTypes]) | |
| ) | |
| const severityRank: Record<ReviewIssueLevel, number> = { | |
| low: 0, | |
| medium: 1, | |
| high: 2, | |
| critical: 3, | |
| } | |
| const normalizedBlockThreshold: ReviewBlockThreshold = | |
| blockThreshold === "none" || | |
| severities.some( | |
| (level) => severityRank[level] >= severityRank[blockThreshold as ReviewIssueLevel] | |
| ) | |
| ? blockThreshold | |
| : "none" | |
| await upsertMutation.mutateAsync({ | |
| repository_id: selectedRepoId, | |
| content, | |
| is_enabled: isEnabled, | |
| severities, | |
| block_threshold: normalizedBlockThreshold, | |
| ignored_file_types: nextIgnoredFileTypes, | |
| summarize_pr_description: summarizePRDescription, | |
| }) | |
| setIgnoredFileTypes(nextIgnoredFileTypes) | |
| setIgnoredFileTypeInput("") | |
| setSavedAt(Date.now()) | |
| } |
| "/repositories/%s/%s/pullrequests/%d", | ||
| workspace, | ||
| ref.Repo, | ||
| ref.Number, |
There was a problem hiding this comment.
PUT with a partial body can overwrite pull request fields or fail validation
This sends a full-resource update request with only the description field populated. For Bitbucket pull requests, PUT semantics commonly require the full current representation; using a partial payload risks clearing unspecified fields (e.g. title, destination/source branches, reviewers, close_source_branch) or being rejected by the API depending on server/cloud behavior. Use a partial-update method if supported, or first GET the existing pull request and send back all required fields merged with the new description.
| var value settingucm.RepositoryRuleValue | ||
| if err := json.Unmarshal([]byte(s.Value), &value); err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Test now accepts empty rule content instead of enforcing prior validation
This change removes coverage for rejecting empty/whitespace-only repository rule content and replaces it with a success case. If the use case is still expected to prevent empty rules, this silently drops a critical business validation and can allow invalid settings to be stored. No safe replacement is possible without confirming intended behavior, but the previous failure-path test should be preserved rather than replaced.
| RepositoryID int64 `json:"repository_id" validate:"required"` | ||
| Content string `json:"content"` | ||
| IsEnabled null.Bool `json:"is_enabled"` | ||
| Severities []constant.ReviewIssueLevel `json:"severities" validate:"omitempty,dive,oneof=low medium high critical"` |
There was a problem hiding this comment.
Rule content is no longer required on upsert requests
Removing validation from Content allows creating or overwriting repository rules with an empty string. Because this struct feeds persisted settings, accepting empty content can silently erase an existing rule or store an unusable rule configuration.
| Severities []constant.ReviewIssueLevel `json:"severities" validate:"omitempty,dive,oneof=low medium high critical"` | |
| Content string `json:"content" validate:"required"` |
| } | ||
| cfg.Severities = value.Severities | ||
| if len(cfg.Severities) == 0 { | ||
| cfg.Severities = append([]constant.ReviewIssueLevel(nil), constant.ReviewAllIssueLevels...) |
There was a problem hiding this comment.
Unvalidated block threshold can fail checks for every issue level
BlockThreshold is accepted from repository settings without validation. If the stored value is invalid or lower than the configured severities, ReviewIssueLevelMeetsThreshold may mark all findings as blocking, causing incorrect failed checks across repositories. Clamp it to a known level and ensure it is compatible with configured severities before returning the config.
| cfg.Severities = append([]constant.ReviewIssueLevel(nil), constant.ReviewAllIssueLevels...) | |
| cfg.BlockThreshold = value.BlockThreshold | |
| if cfg.BlockThreshold == "" || !constant.ReviewIssueLevelIsValid(cfg.BlockThreshold) { | |
| cfg.BlockThreshold = constant.ReviewIssueLevelHigh | |
| } | |
| if len(cfg.Severities) > 0 { | |
| allowed := false | |
| for _, severity := range cfg.Severities { | |
| if severity == cfg.BlockThreshold { | |
| allowed = true | |
| break | |
| } | |
| } | |
| if !allowed { | |
| cfg.BlockThreshold = constant.ReviewIssueLevelHigh | |
| } | |
| } |
| &pullRequest, | ||
| pullRequestRef, | ||
| changedFiles, | ||
| ) |
There was a problem hiding this comment.
LLM summary is written to PR description without sanitization
The generated summary is inserted directly into the PR body and pushed to the Git provider. LLM output can contain arbitrary markdown/HTML, mention spam, or marker strings that break future parsing and can be abused to alter the visible PR description. Sanitize the content and strip reserved markers before composing the new body.
| ) | |
| sanitizedSummary := strings.TrimSpace(result.Content) | |
| sanitizedSummary = strings.ReplaceAll(sanitizedSummary, constant.ReviewPRSummaryStartMarker, "") | |
| sanitizedSummary = strings.ReplaceAll(sanitizedSummary, constant.ReviewPRSummaryEndMarker, "") | |
| sanitizedSummary = strings.ReplaceAll(sanitizedSummary, "@", "＠") | |
| if sanitizedSummary == "" { | |
| logger.Warnf("pr description summarize: empty llm response") | |
| return | |
| } | |
| originalDescription := stripReviewlySummaryBlock(pullRequest.Description) | |
| newDescription := composePRDescription(sanitizedSummary, originalDescription) |
Review SummaryLow overall risk: this diff only adds tests and introduces no production-path changes. No high/critical correctness, security, or performance issues are evident in the added test coverage. Recommended next action: run the full test suite and verify these cases exercise the intended production behavior. Overall risk appears moderate: the change adds severity/file-type filtering but the diff alone shows no clear high/critical correctness or security defect. The main recurring theme is potential behavior drift from new filters. Next action: add/verify tests covering ignored file types and severity-filtered summaries/results end-to-end. No high-severity issues are evident in this diff. The change only adds configuration fields to a model without introducing logic, so the primary risk is deferred to downstream consumers. Next step: verify all construction, validation, and serialization paths handle these new fields safely. Moderate-to-high correctness risk in new file-ignore glob handling. The main recurring theme is custom path/glob matching that can misclassify files across platforms and unintentionally skip review coverage. Next action: replace the ad-hoc glob engine with standard path-aware matching and add cross-platform tests for ignore patterns. High risk: this change removes a key input validation and now persists/returns rule configurations without enforcing basic invariants. The recurring theme is accepting unvalidated user-controlled rule settings. Next step: restore request validation for content and validate enum-like fields before storing them. Overall risk is low in this constants-only change. No high/critical correctness or security issues are evident from the diff alone. Next action: add tests around the new severity-threshold helper, especially for the new 'none' value and unknown levels. Overall risk appears low from this isolated change: it adds a straightforward authenticated PATCH wrapper with no obvious high-severity correctness or security flaw. The main recurring theme is lack of visible guardrails around external API mutations, but nothing here clearly meets high/critical severity. Next action: add/update integration tests covering permissions, empty descriptions, and API error propagation for PR body updates. High risk: the new save path can persist invalid rule configurations that contradict the UI guarantees. The recurring theme is missing normalization/validation before sending state to the API. Next action: enforce payload validation in handleSave so block thresholds and ignore patterns are saved consistently. Low overall risk: this diff only extends an interface and introduces no executable logic. No high/critical issues are evident from the change itself. Next action: ensure all IGitService implementations and mocks are updated and covered by compilation/tests. Overall risk is moderate-to-high due to a likely destructive API usage pattern. The main theme is correctness around partial resource updates potentially overwriting pull request fields. Next step: switch to Bitbucket's supported partial-update semantics or fetch-and-merge the existing PR before updating. Low overall risk: this is generated mock boilerplate and no high/critical correctness, security, or performance issues are evident in the diff. No recurring high-impact theme was found. Next action: run the test suite or regenerate mocks from the interface to confirm signature alignment. Low overall risk from this diff; no high/critical-impact correctness, security, or performance issues are evident in the shown changes. The main theme is filtering behavior consistency, and the next step is to add tests covering ignored-file and severity-filter interactions. Overall risk is low; no high/critical correctness, security, or performance issues are evident in this diff. The main theme is minor accessibility/behavior concerns rather than severe defects. Recommended next action: add interaction and accessibility tests for keyboard navigation and focus management. Low overall risk from this test-only change. The recurring theme is broader behavioral coverage for repository rules and check blocking semantics. Next action: ensure production code has equivalent validation for malformed/unsupported rule values, since these tests currently only cover happy/default paths. Low overall risk from this diff; the main concern is inconsistent filtering semantics that can silently drop expected review outputs. The recurring theme is applying new filters in multiple phases without preserving invariants. Next step: ensure skipped files and severity-filtered empty results are explicitly handled so summaries and attempted counts stay consistent. Moderate-to-high risk: the test changes relax previously enforced validation and may let invalid repository rules persist unnoticed. The recurring theme is converting business-rule validation tests into permissive persistence checks. Next action: verify the use case contract for empty/default rule fields and restore explicit rejection tests if validation is still required. Low overall risk: the change only adds CSS custom variants and introduces no obvious high-severity correctness, security, or performance issues. No recurring high-impact theme was identified. Recommended next action: validate the variants in the build pipeline and component usage with normal testing. Low overall risk from this diff; no high/critical issues are evident in the added method alone. The main theme is standard API update wiring without obvious correctness or security flaws. Recommended next action: validate behavior with integration tests against GitLab for edge cases like empty descriptions and permissions. Low overall risk from this diff alone; it only changes frontend type definitions. The main theme is potential runtime/API compatibility breakage if backend responses or callers don't provide the newly required fields. Next action: verify backend contracts and all call sites, or make new fields optional/defaulted if rollout is incremental. High risk: the request model now accepts empty rule content while adding new fields, which can let callers erase or create invalid repository rules. The recurring theme is weakened validation on persisted configuration inputs. Recommended next action: restore strict validation/defaulting for required fields before these values are stored or enforced. High risk: this change can silently widen repository policy enforcement and mutate PR descriptions from untrusted content. The main recurring theme is missing validation/sanitization of repository rule and LLM-generated data before applying side effects. Next step: validate rule thresholds against configured severities and sanitize/guard PR description updates. Finding breakdown: 10 issue(s): 0 critical, 10 high, 0 medium, 0 low. |
1 participant
AI Summary
Summary
This PR adds structured, per-repository review rules so teams can control which findings are reported, which severities block merges, and which files are ignored during review. It also introduces AI-generated PR description summarization support across Git providers and documents the design/implementation.
Key Changes
diff_only,file_by_file,two_pass) to:highNotable Decisions / Risks
highthreshold to a per-repository setting; defaults preserve prior behavior closely but now also treatcriticalexplicitly.Original Description
Summary
Adds two per-repository configuration features under the Review Rules menu:
1. Structured review rules
high, preserving prior behavior. The block check respects the severity filter..md,*.lock,vendor/**) whose changed files are skipped before review.2. PR Description Summarize
UpdatePullRequestDescriptionimplemented for GitHub, GitLab, and Bitbucket.Both features extend the existing
repository_rulesettings JSON (no new tables/migrations) and the existing/web/rulesscreen. Defaults preserve current behavior for existing repositories.Test Plan
go test ./internal/usecase/setting/... ./internal/services/review/... ./internal/usecase/review/... ./internal/services/git/...— 270 passedmake cover— 75.7% (above 60% gate)make lint— 0 issuesmake build-web— passedmake mock— regenerated IGitService mockUltraworked with Sisyphus