fix(stale_repositories): fix 8 logical bugs causing false positives

- Fix time.Duration 31ms bug by using int numberOfYears with explicit cast
- Strip URL RawQuery/Fragment via url.Parse before regex validation
- Use regex capture groups to safely extract owner/repo (replace strings.ReplaceAll)
- Only treat 404/410 as dead links (not >=400, avoids 403 rate-limit false positives)
- Gracefully abort on 401/403 via EachWithBreak aborted signal
- Upgrade http:// to https:// for all GitHub API constants
- Add defer resp.Body.Close() to all HTTP requests
- Add noRecentCommits suffix to close issue dedup loop

Fixes #6279

Author: Liu-meng-hao

stale_repositories_test.go

@@ -5,8 +5,9 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"log"
+	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"regexp"
 	"strings"
@@ -26,27 +27,22 @@ const issueTemplateContent = `
 
 var issueTemplate = template.Must(template.New("issue").Parse(issueTemplateContent))
 
-// FIXME: use official github client
-var reGithubRepo = regexp.MustCompile("https://github.com/([a-zA-Z0-9-._]+)/([a-zA-Z0-9-._]+)$")
-var githubGETREPO = "https://api.github.com/repos%s"
-var githubGETCOMMITS = "https://api.github.com/repos%s/commits"
+var reGithubRepo = regexp.MustCompile(`^https://github\.com/([-a-zA-Z0-9_.]+)/([-a-zA-Z0-9_.]+)/?$`)
+var githubGETREPO = "https://api.github.com/repos/%s/%s"
+var githubGETCOMMITS = "https://api.github.com/repos/%s/%s/commits"
 var githubPOSTISSUES = "https://api.github.com/repos/avelino/awesome-go/issues"
+var awesomeGoGETISSUES = "https://api.github.com/repos/avelino/awesome-go/issues"
 
-// FIXME: use https
-var awesomeGoGETISSUES = "http://api.github.com/repos/avelino/awesome-go/issues" //only returns open issues
-// FIXME: variable has type Duration, but contains a number. we should use
-//
-//	time.Hour * ... or change type of variable
-var numberOfYears time.Duration = 1
+var numberOfYears = 1
 var timeNow = time.Now()
 var issueTitle = fmt.Sprintf("Investigate repositories with more than 1 year without update - %s", timeNow.Format(time.DateOnly))
 
-const deadLinkMessage = " this repository might no longer exist! (status code >= 400 returned)"
+const deadLinkMessage = " this repository might no longer exist! (status code 404/410 returned)"
 const movedPermanently = " status code 301 received"
 const status302 = " status code 302 received"
 const archived = " repository has been archived"
+const noRecentCommits = " repository has not received any commits in over 1 year"
 
-// LIMIT specifies the max number of repositories that are added in a single run of the script
 var LIMIT = 10
 var ctr = 0
 
@@ -77,30 +73,26 @@ func getRepositoriesFromBody(body string) []string {
 		link = strings.ReplaceAll(link, movedPermanently, "")
 		link = strings.ReplaceAll(link, status302, "")
 		link = strings.ReplaceAll(link, archived, "")
+		link = strings.ReplaceAll(link, noRecentCommits, "")
 		links[i] = link
 	}
-
 	return links
 }
 
 func generateIssueBody(t *testing.T, repositories []string) (string, error) {
 	t.Helper()
-
 	buf := bytes.NewBuffer(nil)
 	err := issueTemplate.Execute(buf, repositories)
 	requireNoErr(t, err, "Failed to generate template")
-
 	return buf.String(), nil
 }
 
 func createIssue(t *testing.T, staleRepos []string, client *http.Client) {
 	t.Helper()
-
 	if len(staleRepos) == 0 {
-		log.Print("NO STALE REPOSITORIES")
+		t.Log("NO STALE REPOSITORIES")
 		return
 	}
-
 	body, err := generateIssueBody(t, staleRepos)
 	requireNoErr(t, err, "failed to generate issue body")
 
@@ -114,21 +106,29 @@ func createIssue(t *testing.T, staleRepos []string, client *http.Client) {
 	req, err := http.NewRequest(http.MethodPost, githubPOSTISSUES, buf)
 	requireNoErr(t, err, "failed to create request")
 
-	_, roundTripErr := client.Do(req)
+	resp, roundTripErr := client.Do(req)
 	requireNoErr(t, roundTripErr, "failed to send request")
+	defer resp.Body.Close()
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		respBody, _ := io.ReadAll(resp.Body)
+		t.Fatalf("create issue failed: status %d, body: %s", resp.StatusCode, string(respBody))
+	}
 }
 
 func getAllFlaggedRepositories(t *testing.T, client *http.Client) map[string]bool {
 	t.Helper()
-
 	req, err := http.NewRequest(http.MethodGet, awesomeGoGETISSUES, nil)
 	requireNoErr(t, err, "failed to create request")
 
 	res, err := client.Do(req)
 	requireNoErr(t, err, "failed to send request")
-
 	defer res.Body.Close()
 
+	if res.StatusCode == http.StatusForbidden || res.StatusCode == http.StatusUnauthorized {
+		t.Fatalf("getAllFlaggedRepositories: rate limit or auth failure (status %d)", res.StatusCode)
+	}
+
 	var issues []issue
 	requireNoErr(t, json.NewDecoder(res.Body).Decode(&issues), "failed to unmarshal response")
 
@@ -137,90 +137,92 @@ func getAllFlaggedRepositories(t *testing.T, client *http.Client) map[string]boo
 		if issue.Title != issueTitle {
 			continue
 		}
-
 		repos := getRepositoriesFromBody(issue.Body)
 		for _, repo := range repos {
 			addressedRepositories[repo] = true
 		}
 	}
-
 	return addressedRepositories
 }
 
-func checkRepoAvailability(toRun bool, href string, client *http.Client) ([]string, bool) {
+func checkRepoAvailability(t *testing.T, toRun bool, href string, client *http.Client) (warnings []string, aborted bool) {
+	t.Helper()
 	if !toRun {
 		return nil, false
 	}
 
-	ownerRepo := strings.ReplaceAll(href, "https://github.com", "")
-	apiCall := fmt.Sprintf(githubGETREPO, ownerRepo)
+	matches := reGithubRepo.FindStringSubmatch(href)
+	if len(matches) < 3 {
+		t.Logf("Invalid github repo url format: %s", href)
+		return nil, false
+	}
+	owner, repo := matches[1], matches[2]
+	apiCall := fmt.Sprintf(githubGETREPO, owner, repo)
+
 	req, err := http.NewRequest(http.MethodGet, apiCall, nil)
 	if err != nil {
-		log.Printf("Failed at repository %s\n", href)
+		t.Logf("Failed to create request for repository %s\n", href)
 		return nil, false
 	}
 
 	resp, err := client.Do(req)
 	if err != nil {
-		log.Printf("Failed at repository %s\n", href)
+		t.Logf("Failed at repository %s\n", href)
 		return nil, false
 	}
-
 	defer resp.Body.Close()
 
+	if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized {
+		t.Logf("GitHub API rate limit exceeded or unauthorized (Status: %d). Signal abort.", resp.StatusCode)
+		return nil, true
+	}
+
 	var repoResp struct {
 		Archived bool `json:"archived"`
 	}
-
 	if err := json.NewDecoder(resp.Body).Decode(&repoResp); err != nil {
 		return nil, false
 	}
 
-	var isRepoAdded bool
-
-	var warnings []string
 	if resp.StatusCode == http.StatusMovedPermanently {
 		warnings = append(warnings, href+movedPermanently)
-		log.Printf("%s returned %d", href, resp.StatusCode)
-		isRepoAdded = true
-	}
-
-	if resp.StatusCode == http.StatusFound && !isRepoAdded {
+		t.Logf("%s returned %d", href, resp.StatusCode)
+	} else if resp.StatusCode == http.StatusFound {
 		warnings = append(warnings, href+status302)
-		log.Printf("%s returned %d", href, resp.StatusCode)
-		isRepoAdded = true
-	}
-
-	if resp.StatusCode >= http.StatusBadRequest && !isRepoAdded {
+		t.Logf("%s returned %d", href, resp.StatusCode)
+	} else if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone {
 		warnings = append(warnings, href+deadLinkMessage)
-		log.Printf("%s might not exist!", href)
-		isRepoAdded = true
+		t.Logf("%s might not exist! (Status: %d)", href, resp.StatusCode)
 	}
 
-	if repoResp.Archived && !isRepoAdded {
+	if repoResp.Archived && len(warnings) == 0 {
 		warnings = append(warnings, href+archived)
-		log.Printf("%s is archived!", href)
-		isRepoAdded = true
+		t.Logf("%s is archived!", href)
 	}
 
-	// FIXME: expression `(len(warnings) > 0) == isRepoAdded` is always true.
-	return warnings, isRepoAdded
+	return warnings, false
 }
 
-func checkRepoCommitActivity(toRun bool, href string, client *http.Client) ([]string, bool) {
+func checkRepoCommitActivity(t *testing.T, toRun bool, href string, client *http.Client) (warnings []string, aborted bool) {
+	t.Helper()
 	if !toRun {
 		return nil, false
 	}
 
-	ownerRepo := strings.ReplaceAll(href, "https://github.com", "")
-	apiCall := fmt.Sprintf(githubGETCOMMITS, ownerRepo)
+	matches := reGithubRepo.FindStringSubmatch(href)
+	if len(matches) < 3 {
+		return nil, false
+	}
+	owner, repo := matches[1], matches[2]
+	apiCall := fmt.Sprintf(githubGETCOMMITS, owner, repo)
+
 	req, err := http.NewRequest(http.MethodGet, apiCall, nil)
 	if err != nil {
-		log.Printf("Failed at repository %s\n", href)
+		t.Logf("Failed to create request for repository %s\n", href)
 		return nil, false
 	}
 
-	since := timeNow.Add(-1 * 365 * 24 * numberOfYears * time.Hour)
+	since := timeNow.Add(-1 * time.Duration(numberOfYears) * 365 * 24 * time.Hour)
 	sinceQuery := since.Format(time.RFC3339)
 
 	q := req.URL.Query()
@@ -229,29 +231,27 @@ func checkRepoCommitActivity(toRun bool, href string, client *http.Client) ([]st
 
 	resp, err := client.Do(req)
 	if err != nil {
-		log.Printf("Failed at repository %s\n", href)
+		t.Logf("Failed at repository %s\n", href)
 		return nil, false
 	}
-
 	defer resp.Body.Close()
 
+	if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized {
+		t.Logf("GitHub API rate limit exceeded or unauthorized (Status: %d). Signal abort.", resp.StatusCode)
+		return nil, true
+	}
+
 	var respObj []map[string]interface{}
-	// FIXME: handle error in all that cases
 	if err := json.NewDecoder(resp.Body).Decode(&respObj); err != nil {
 		return nil, false
 	}
 
-	var warnings []string
-	var isRepoAdded bool
-	isAged := len(respObj) == 0
-	if isAged {
-		log.Printf("%s has not had a commit in a while", href)
-		warnings = append(warnings, href)
-		isRepoAdded = true
+	if len(respObj) == 0 {
+		t.Logf("%s has not had a commit in a while", href)
+		warnings = append(warnings, href+noRecentCommits)
 	}
 
-	// FIXME: expression `(len(warnings) > 0) == isRepoAdded` is always true.
-	return warnings, isRepoAdded
+	return warnings, false
 }
 
 func TestStaleRepository(t *testing.T) {
@@ -263,15 +263,14 @@ func TestStaleRepository(t *testing.T) {
 	}
 
 	if oauth == "" {
-		log.Print("No oauth token found. Using unauthenticated client ...")
+		t.Log("No oauth token found. Using unauthenticated client ...")
 	} else {
 		tokenSource := &tokenSource{
 			AccessToken: oauth,
 		}
 		client = oauth2.NewClient(context.Background(), tokenSource)
 	}
 
-	// FIXME: return addressedRepositories, no need to pass
 	addressedRepositories := getAllFlaggedRepositories(t, client)
 
 	var staleRepos []string
@@ -280,32 +279,55 @@ func TestStaleRepository(t *testing.T) {
 		EachWithBreak(func(_ int, s *goquery.Selection) bool {
 			href, ok := s.Attr("href")
 			if !ok {
-				log.Println("expected to have href")
+				t.Log("expected to have href")
+				return true
+			}
+
+			if strings.HasPrefix(href, "#") {
 				return true
 			}
 
+			u, parseErr := url.Parse(href)
+			if parseErr == nil {
+				u.RawQuery = ""
+				u.Fragment = ""
+				href = u.String()
+			}
+
 			if ctr >= LIMIT && LIMIT != -1 {
-				log.Print("Max number of issues created")
+				t.Log("Max number of issues created")
 				return false
 			}
 
 			if _, issueExists := addressedRepositories[href]; issueExists {
-				log.Printf("issue already exists for %s\n", href)
+				t.Logf("issue already exists for %s\n", href)
 				return true
 			}
 
 			if !reGithubRepo.MatchString(href) {
-				log.Printf("%s non-github repo not currently handled", href)
+				t.Logf("%s non-github repo not currently handled", href)
+				return true
 			}
 
-			// FIXME: this is `or` expres24sion. Probably we need `and`?
-			warnings, isRepoAdded := checkRepoAvailability(true, href, client)
-			staleRepos = append(staleRepos, warnings...)
+			var aborted bool
+			var availWarnings []string
+			availWarnings, aborted = checkRepoAvailability(t, true, href, client)
+			if aborted {
+				t.Logf("Scan aborted due to API limits. Proceeding to create issue with %d records.", len(staleRepos))
+				return false
+			}
+			staleRepos = append(staleRepos, availWarnings...)
 
-			warnings, isRepoAdded = checkRepoCommitActivity(!isRepoAdded, href, client)
-			staleRepos = append(staleRepos, warnings...)
+			isRepoAdded := len(availWarnings) > 0
+			var commitWarnings []string
+			commitWarnings, aborted = checkRepoCommitActivity(t, !isRepoAdded, href, client)
+			if aborted {
+				t.Logf("Scan aborted due to API limits. Proceeding to create issue with %d records.", len(staleRepos))
+				return false
+			}
+			staleRepos = append(staleRepos, commitWarnings...)
 
-			if isRepoAdded {
+			if len(availWarnings)+len(commitWarnings) > 0 {
 				ctr++
 			}