feat: sitemap-aware batch conflict and the sitemap_blocked issue

- noindex_published fires only when a sitemap file lists the address now (read once per batch through contract.Listing) - CMS issue sitemap_blocked (severity error, tool seo_update_robots, details line and rule): a listed address the production robots.txt keeps Googlebot from - the SEO tab's P1 shows status.excluded_title with the reason (noindex, canonical elsewhere, redirect, or the type switched off in the sitemap settings via ListingSettings.TypesOff) as a plain callout - robots.txt is composed once per check of many addresses
2026-11-04 13:39:31 +01:00
parent 742a5b930b
commit 59f0457c69
11 changed files with 370 additions and 22 deletions
--- a/classes/batch.go
+++ b/classes/batch.go
@@ -290,8 +290,9 @@ func (s *Service) runBatch(ctx context.Context, opts BatchOptions, build func(ct
 			}
 			res.Changes = append(res.Changes, item)
 		}
+		listed := &listedOnce{svc: s}
 		for i := range res.Changes {
-			late, err := s.batchConflicts(ctx, tx, i, changes[i], res.Changes[i])
+			late, err := s.batchConflicts(ctx, tx, listed, i, changes[i], res.Changes[i])
 			if err != nil {
 				return err
 			}
@@ -452,12 +453,34 @@ func (s *Service) applyChange(ctx context.Context, tx *gorm.DB, index int, ch Ba
 	return errUnknownOp
 }

+// listedOnce reads the addresses the sitemaps list once per batch.
+type listedOnce struct {
+	svc    *Service
+	read   bool
+	ok     bool
+	listed map[string]bool
+}
+
+// has reports whether the sitemaps list address; ok is false without the
+// sitemap plugin's Listing.
+func (l *listedOnce) has(ctx context.Context, address string) (listed, ok bool, err error) {
+	if !l.read {
+		l.listed, l.ok, err = l.svc.SitemapListed(context.WithoutCancel(ctx))
+		if err != nil {
+			return false, false, err
+		}
+		l.read = true
+	}
+	return l.listed[address], l.ok, nil
+}
+
 // batchConflicts are the warnings that depend on the whole batch, checked
 // once every change is in place: noindex on the address of a published
-// record (the records the sitemaps list; Phase 33's sitemap filter
-// refines it) and a canonical pointing to a redirected or noindex
-// address.
-func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, index int, ch BatchChange, item BatchItem) ([]Conflict, error) {
+// record that a sitemap file lists now (read through contract.Listing, so
+// an address the sitemaps already leave out, such as one of a content
+// type switched off, is no conflict; D-12, D-24), and a canonical pointing
+// to a redirected or noindex address.
+func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, listed *listedOnce, index int, ch BatchChange, item BatchItem) ([]Conflict, error) {
 	if item.Error != "" {
 		return nil, nil
 	}
@@ -490,7 +513,15 @@ func (s *Service) batchConflicts(ctx context.Context, tx *gorm.DB, index int, ch
 		if err != nil && !errors.Is(err, ErrBadURL) {
 			return nil, err
 		}
-		if err == nil && owner.WrittenTo == WrittenToRecord && owner.Published {
+		inSitemap := err == nil && owner.WrittenTo == WrittenToRecord && owner.Published
+		if inSitemap {
+			has, ok, lerr := listed.has(ctx, owner.URL)
+			if lerr != nil {
+				return nil, lerr
+			}
+			inSitemap = !ok || has
+		}
+		if inSitemap {
 			out = append(out, Conflict{Change: index, Kind: ConflictNoindexPublished, URL: owner.URL,
 				Message: s.PhraseCtx(ctx, "golem15.seo::lang.api.conflict_noindex_published", map[string]string{"url": owner.URL})})
 		}
--- a/classes/issues.go
+++ b/classes/issues.go
@@ -21,6 +21,15 @@ const (
 	IssueCanonicalNoindex    = "canonical_noindex"
 	IssueCanonicalRedirected = "canonical_redirected"
 	IssueNoindexBlocked      = "noindex_blocked"
+	// IssueSitemapBlocked is a sitemap address the production robots.txt
+	// keeps Googlebot from (D-24): an error, since the sitemap asks for a
+	// page the crawler may not fetch.
+	IssueSitemapBlocked = "sitemap_blocked"
+)
+
+// The severities an issue may name.
+const (
+	SeverityError = "error"
 )

 // Issue is one finding of the issue report: the address, the kind, the
@@ -31,6 +40,9 @@ type Issue struct {
 	Locale  string         `json:"locale,omitempty"`
 	Details map[string]any `json:"details,omitempty"`
 	Tool    string         `json:"tool"`
+	// Severity is set for kinds that are errors (sitemap_blocked); "" for
+	// the others.
+	Severity string `json:"severity,omitempty"`
 }

 // The tools the issues point to (pier prefixes them with seo_).
@@ -71,6 +83,14 @@ func (s *Service) Issues(ctx context.Context) ([]Issue, error) {
 	if data.noindex, err = noindexAddresses(db); err != nil {
 		return nil, err
 	}
+	listed, _, err := s.SitemapListed(ctx)
+	if err != nil {
+		return nil, err
+	}
+	for address := range listed {
+		data.listed = append(data.listed, address)
+	}
+	sort.Strings(data.listed)
 	return s.issuesOf(ctx, data, s.canonicalProblem)
 }

@@ -85,6 +105,8 @@ type issueData struct {
 	redirects []models.Redirect
 	robots    models.Robots
 	noindex   []string
+	// listed are the addresses the sitemaps list (path with query).
+	listed []string
 }

 // issuesOf builds the report from data; canonical classifies a canonical
@@ -228,10 +250,22 @@ func (s *Service) issuesOf(ctx context.Context, data issueData, canonical func(c
 	}

 	// Noindex addresses robots.txt blocks.
+	file := ComposeRobots(data.robots.Content, data.robots.Bots, s.SitemapLines(ctx))
 	for _, u := range s.blockedNoindex(data.noindex, data.robots.Content, data.robots.Bots) {
 		out = append(out, Issue{Kind: IssueNoindexBlocked, URL: u, Tool: toolUpdateRobots})
 	}

+	// Sitemap addresses the production robots.txt blocks for Googlebot.
+	for _, u := range data.listed {
+		test := s.testRobotsFile(file, data.robots.Content, "Googlebot", u)
+		if test.Allowed {
+			continue
+		}
+		out = append(out, Issue{Kind: IssueSitemapBlocked, URL: u, Tool: toolUpdateRobots, Severity: SeverityError, Details: map[string]any{
+			"line": test.Line, "rule": test.Rule,
+		}})
+	}
+
 	sort.SliceStable(out, func(i, j int) bool {
 		if out[i].Kind != out[j].Kind {
 			return out[i].Kind < out[j].Kind
--- a/classes/partials.go
+++ b/classes/partials.go
@@ -8,6 +8,7 @@ import (
 	"strings"

 	"git.golem15.com/golem15/sm-seo-plugin/models"
+	"git.golem15.com/golem15/sm-sitemap-plugin/contract"
 	"gorm.io/gorm"
 )

@@ -15,10 +16,12 @@ import (
 // the request locale), never a GORM model; html/template escapes every
 // value.

-// Callout is one P1 warning.
+// Callout is one P1 callout: a warning, or a plain note when Plain is
+// set (the sitemap status).
 type Callout struct {
 	Title string
 	Text  string
+	Plain bool
 }

 // StatusView is the P1 seo_status view model.
@@ -202,9 +205,33 @@ func (s *Service) RecordStatus(ctx context.Context, key, id string) (StatusView,
 			}
 		}
 	}
+	exclusions, err := s.RecordSitemapExclusions(ctx, key, id)
+	if err != nil {
+		return view, err
+	}
+	for _, x := range exclusions {
+		key, ok := excludedTexts[x.Reason]
+		if !ok {
+			continue
+		}
+		view.Callouts = append(view.Callouts, Callout{
+			Title: s.PhraseCtx(ctx, "golem15.seo::lang.status.excluded_title", nil),
+			Text:  s.PhraseCtx(ctx, key, map[string]string{"url": x.Target}),
+			Plain: true,
+		})
+	}
 	return view, nil
 }

+// excludedTexts are the P1 texts of the sitemap exclusion reasons (UI-SPEC
+// C16).
+var excludedTexts = map[string]string{
+	contract.ReasonNoindex:   "golem15.seo::lang.status.excluded_noindex",
+	contract.ReasonCanonical: "golem15.seo::lang.status.excluded_canonical",
+	contract.ReasonRedirect:  "golem15.seo::lang.status.excluded_redirect",
+	contract.ReasonType:      "golem15.seo::lang.status.excluded_type",
+}
+
 // ruleCallout is the override callout of the first rule that sets a value.
 func (s *Service) ruleCallout(ctx context.Context, rules []models.Rule) (Callout, bool) {
 	for _, r := range rules {
--- a/classes/robots.go
+++ b/classes/robots.go
@@ -205,7 +205,12 @@ type RobotsTest struct {
 // grobotstxt, Google's matcher (D-22, D-99): longest match, Allow wins a
 // tie, * and $.
 func (s *Service) TestRobots(content string, bots models.BotSwitches, agent, path string) RobotsTest {
-	file := ComposeRobots(content, bots, s.SitemapLines(context.Background()))
+	return s.testRobotsFile(ComposeRobots(content, bots, s.SitemapLines(context.Background())), content, agent, path)
+}
+
+// testRobotsFile is TestRobots on a composed file; content is the editor's
+// part of it (the generated bot rules follow it).
+func (s *Service) testRobotsFile(file, content, agent, path string) RobotsTest {
 	uri := AbsoluteURL(s.SiteURL(), path)
 	if s.SiteURL() == "" {
 		uri = "http://localhost" + path
@@ -269,8 +274,12 @@ func (s *Service) BlockedNoindex(db *gorm.DB, content string, bots models.BotSwi
 // the production file composed from content and bots.
 func (s *Service) blockedNoindex(urls []string, content string, bots models.BotSwitches) []string {
 	var out []string
+	if len(urls) == 0 {
+		return out
+	}
+	file := ComposeRobots(content, bots, s.SitemapLines(context.Background()))
 	for _, u := range urls {
-		if !s.TestRobots(content, bots, "Googlebot", u).Allowed {
+		if !s.testRobotsFile(file, content, "Googlebot", u).Allowed {
 			out = append(out, u)
 		}
 	}
--- a/classes/sitemap_filter.go
+++ b/classes/sitemap_filter.go
@@ -132,23 +132,31 @@ func (s *Service) newFilterBatch(ctx context.Context) (*filterBatch, error) {
 	}, nil
 }

-// decide is the decision of one candidate: a redirected address first (it
-// is no page), then noindex, then a canonical URL elsewhere.
+// decide is the decision of one candidate.
 func (b *filterBatch) decide(ctx context.Context, c contract.Candidate) (contract.Decision, error) {
+	d, _, err := b.decideTarget(ctx, c)
+	return d, err
+}
+
+// decideTarget is the decision of one candidate and, for a canonical or a
+// redirect, the address the sitemaps would list instead: a redirected
+// address first (it is no page), then noindex, then a canonical URL
+// elsewhere.
+func (b *filterBatch) decideTarget(ctx context.Context, c contract.Candidate) (contract.Decision, string, error) {
 	path, query, rawQuery, ok := b.candidatePath(c.URL)
 	if !ok {
-		return contract.Decision{}, nil
+		return contract.Decision{}, "", nil
 	}
 	norm, changed := Normalize(&url.URL{Path: path, RawQuery: rawQuery}, b.opts)
 	if changed {
-		return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, nil
+		return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, joinQuery(norm.Path, norm.RawQuery), nil
 	}
 	chain, err := b.set.Follow(b.site, norm.Path, norm.RawQuery, MaxRedirectHops)
 	if err != nil {
-		return contract.Decision{}, err
+		return contract.Decision{}, "", err
 	}
 	if len(chain.Hops) > 0 {
-		return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, nil
+		return contract.Decision{Exclude: true, Reason: contract.ReasonRedirect}, displayURL(b.site, chain.Final), nil
 	}
 	key := ""
 	var match *pact.ContentMatchResult
@@ -160,20 +168,121 @@ func (b *filterBatch) decide(ctx context.Context, c contract.Candidate) (contrac
 	}
 	res, err := b.resolver.resolveAt(ctx, b.db, b.settings, path, query, rawQuery, key, match, nil, true)
 	if err != nil {
-		return contract.Decision{}, err
+		return contract.Decision{}, "", err
 	}
 	if res.SEO == nil {
-		return contract.Decision{}, nil
+		return contract.Decision{}, "", nil
 	}
 	if r, err := ParseRobots(res.SEO.Robots); err == nil && r.Index == "noindex" {
-		return contract.Decision{Exclude: true, Reason: contract.ReasonNoindex}, nil
+		return contract.Decision{Exclude: true, Reason: contract.ReasonNoindex}, "", nil
 	}
 	if !sameAddress(res.SEO.Canonical, res.URL) {
-		return contract.Decision{Exclude: true, Reason: contract.ReasonCanonical}, nil
+		return contract.Decision{Exclude: true, Reason: contract.ReasonCanonical}, displayURL(b.site, res.SEO.Canonical), nil
 	}
-	return contract.Decision{}, nil
+	return contract.Decision{}, "", nil
 }

+// SitemapExclusion is why a record's address is left out of the sitemaps:
+// a contract reason and, for canonical and redirect, the address listed
+// instead.
+type SitemapExclusion struct {
+	Locale string
+	Reason string
+	Target string
+}
+
+// RecordSitemapExclusions lists why the sitemaps leave out the addresses
+// of record id of content type key, one per reason (the first locale that
+// has it): the type switched off in the sitemap settings (read through
+// contract.Listing), else the SEO plugin's own decision on each address
+// (noindex, canonical elsewhere, redirect). Nothing for an unpublished
+// record: the sitemaps never list it.
+func (s *Service) RecordSitemapExclusions(ctx context.Context, key, id string) ([]SitemapExclusion, error) {
+	reg := s.Registry()
+	if reg == nil {
+		return nil, nil
+	}
+	rec, err := reg.Describe(ctx, key, id, s.UnprefixedLocale())
+	if err != nil || rec == nil || !rec.Published {
+		return nil, err
+	}
+	if l := s.listing(); l != nil {
+		st, err := l.Settings(ctx)
+		if err != nil {
+			return nil, err
+		}
+		for _, off := range st.TypesOff {
+			if off == key {
+				return []SitemapExclusion{{Reason: contract.ReasonType}}, nil
+			}
+		}
+	}
+	b, err := s.newFilterBatch(ctx)
+	if err != nil {
+		return nil, err
+	}
+	var out []SitemapExclusion
+	seen := map[string]bool{}
+	for _, l := range rec.Locales {
+		if strings.TrimSpace(l.URL) == "" {
+			continue
+		}
+		d, target, err := b.decideTarget(ctx, contract.Candidate{TypeKey: key, RecordID: id, Locale: l.Locale, URL: l.URL})
+		if err != nil {
+			return nil, err
+		}
+		if d.Exclude && !seen[d.Reason] {
+			seen[d.Reason] = true
+			out = append(out, SitemapExclusion{Locale: l.Locale, Reason: d.Reason, Target: target})
+		}
+	}
+	return out, nil
+}
+
+// SitemapListed is the set of addresses the sitemaps list, read through
+// contract.Listing: their paths with the query, as the owner of a URL
+// names them. ok is false without a published Listing. At most
+// MaxListedAddresses are read.
+func (s *Service) SitemapListed(ctx context.Context) (map[string]bool, bool, error) {
+	l := s.listing()
+	if l == nil {
+		return nil, false, nil
+	}
+	st, err := l.Settings(ctx)
+	if err != nil {
+		return nil, false, err
+	}
+	out := map[string]bool{}
+	read := 0
+	for _, g := range st.Groups {
+		after := ""
+		for read < MaxListedAddresses {
+			page, err := l.Entries(ctx, contract.EntriesQuery{Group: g.Key, After: after, Limit: 1000})
+			if err != nil {
+				return nil, false, err
+			}
+			for _, e := range page.Entries {
+				read++
+				if u, err := url.Parse(e.URL); err == nil {
+					path := u.Path
+					if path == "" {
+						path = "/"
+					}
+					out[joinQuery(path, u.RawQuery)] = true
+				}
+			}
+			if page.Next == "" || page.Next == after {
+				break
+			}
+			after = page.Next
+		}
+	}
+	return out, true, nil
+}
+
+// MaxListedAddresses caps the sitemap addresses SitemapListed reads.
+const MaxListedAddresses = 100000
+
 // candidatePath turns a candidate's address (a path, or an absolute URL on
 // the site or its www twin) into the decoded path and its query; ok is
 // false for an address the plugin cannot judge (another host, malformed).