fix: address review feedback on PR #106
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m42s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m4s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m42s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m4s
- Replace interface{} with any in github/reviews.go (Go 1.18+ idiom)
- Add default panic case to VCS client init switch
- Refactor supersedeOldReviews to return error instead of os.Exit(1)
- Remove spurious blank lines in formatter.go and formatter_test.go
- Add doc comment to DeleteReview explaining when to use vs DismissReview
- Sanitize extractSentinelName output to prevent log injection
This commit is contained in:
+25
-6
@@ -169,6 +169,8 @@ func main() {
|
|||||||
ghBaseURL = "https://api.github.com"
|
ghBaseURL = "https://api.github.com"
|
||||||
}
|
}
|
||||||
client = github.NewClient(*reviewerToken, ghBaseURL)
|
client = github.NewClient(*reviewerToken, ghBaseURL)
|
||||||
|
default:
|
||||||
|
panic("unreachable: unhandled provider " + *provider)
|
||||||
}
|
}
|
||||||
slog.Info("VCS client initialized", "provider", *provider)
|
slog.Info("VCS client initialized", "provider", *provider)
|
||||||
|
|
||||||
@@ -498,7 +500,10 @@ func main() {
|
|||||||
|
|
||||||
// Supersede all old reviews
|
// Supersede all old reviews
|
||||||
if len(oldReviews) > 0 {
|
if len(oldReviews) > 0 {
|
||||||
supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel)
|
if err := supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel); err != nil {
|
||||||
|
slog.Error("failed to supersede old reviews", "error", err)
|
||||||
|
os.Exit(1)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -517,7 +522,7 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
|
|||||||
// supersedeOldReviews marks old reviews as superseded.
|
// supersedeOldReviews marks old reviews as superseded.
|
||||||
// For GitHub: dismisses old reviews.
|
// For GitHub: dismisses old reviews.
|
||||||
// For Gitea: edits the review body and resolves inline comments.
|
// For Gitea: edits the review body and resolves inline comments.
|
||||||
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) {
|
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) error {
|
||||||
if provider == "github" {
|
if provider == "github" {
|
||||||
for _, old := range oldReviews {
|
for _, old := range oldReviews {
|
||||||
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
|
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
|
||||||
@@ -526,14 +531,13 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
|
|||||||
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
|
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// Gitea: existing EditComment + ResolveComment flow
|
// Gitea: existing EditComment + ResolveComment flow
|
||||||
giteaAdapter, ok := client.(*gitea.Adapter)
|
giteaAdapter, ok := client.(*gitea.Adapter)
|
||||||
if !ok {
|
if !ok {
|
||||||
slog.Error("expected gitea.Adapter for gitea provider")
|
return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client)
|
||||||
os.Exit(1)
|
|
||||||
}
|
}
|
||||||
underlying := giteaAdapter.Underlying()
|
underlying := giteaAdapter.Underlying()
|
||||||
|
|
||||||
@@ -576,6 +580,7 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
|
|||||||
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
|
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// fetchFileContext fetches the full content of modified files from the PR branch.
|
// fetchFileContext fetches the full content of modified files from the PR branch.
|
||||||
@@ -846,7 +851,21 @@ func extractSentinelName(body string) string {
|
|||||||
if end < 0 {
|
if end < 0 {
|
||||||
return "unknown"
|
return "unknown"
|
||||||
}
|
}
|
||||||
return rest[:end]
|
name := rest[:end]
|
||||||
|
// Sanitize: strip control characters to prevent log injection.
|
||||||
|
name = strings.Map(func(r rune) rune {
|
||||||
|
if r < 0x20 || r == 0x7f {
|
||||||
|
return -1
|
||||||
|
}
|
||||||
|
return r
|
||||||
|
}, name)
|
||||||
|
if len(name) > 64 {
|
||||||
|
name = name[:64]
|
||||||
|
}
|
||||||
|
if name == "" {
|
||||||
|
return "unknown"
|
||||||
|
}
|
||||||
|
return name
|
||||||
}
|
}
|
||||||
|
|
||||||
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
||||||
|
|||||||
+4
-2
@@ -141,7 +141,9 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
|
|||||||
return allReviews, nil
|
return allReviews, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// DeleteReview deletes a review from a pull request.
|
// DeleteReview permanently deletes a review from a pull request.
|
||||||
|
// Use DismissReview instead when the review should remain visible but marked as dismissed
|
||||||
|
// (e.g., superseding an outdated review while preserving history).
|
||||||
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
||||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
|
||||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
||||||
@@ -185,7 +187,7 @@ func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
|||||||
|
|
||||||
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
|
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
|
||||||
// It handles HTTPS validation, authentication, and response reading.
|
// It handles HTTPS validation, authentication, and response reading.
|
||||||
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload interface{}) ([]byte, error) {
|
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
|
||||||
const maxErrorBodyBytes = 4 * 1024
|
const maxErrorBodyBytes = 4 * 1024
|
||||||
|
|
||||||
jsonBody, err := json.Marshal(payload)
|
jsonBody, err := json.Marshal(payload)
|
||||||
|
|||||||
@@ -10,7 +10,6 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
|
|||||||
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
|
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
|
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
|
||||||
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
||||||
// Persona display names are controlled by repo owners (trusted input).
|
// Persona display names are controlled by repo owners (trusted input).
|
||||||
|
|||||||
@@ -98,7 +98,6 @@ func TestFormatMarkdown_SpecialChars(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
||||||
result := &ReviewResult{
|
result := &ReviewResult{
|
||||||
Verdict: "APPROVE",
|
Verdict: "APPROVE",
|
||||||
|
|||||||
Reference in New Issue
Block a user