From 133c3cfd1c5cb73afbcea93b6852bb1fe00227d4 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 4 Nov 2026 21:26:39 +0100 Subject: [PATCH] docs(33): pre-release code review of phase 33 (four lanes) --- .../33-REVIEW.md | 1189 +++++++++++++++++ 1 file changed, 1189 insertions(+) create mode 100644 .planning/phases/33-seo-output-audit-and-content/33-REVIEW.md diff --git a/.planning/phases/33-seo-output-audit-and-content/33-REVIEW.md b/.planning/phases/33-seo-output-audit-and-content/33-REVIEW.md new file mode 100644 index 00000000..cb6862ac --- /dev/null +++ b/.planning/phases/33-seo-output-audit-and-content/33-REVIEW.md @@ -0,0 +1,1189 @@ +--- +phase: 33 +reviewed: 2026-11-04 +depth: standard +lanes: 4 +findings: + critical: 4 + warning: 29 + info: 36 + total: 69 +status: issues_found +--- + +# Phase 33 code review + +Pre-release review of every repository changed in phase 33, before any tag. There are four lanes: +1. framework and sm-journal-plugin +2. sm-sitemap-plugin +3. sm-seo-plugin +4. the Nuxt layers + +Finding IDs are per lane, prefixed L1 to L4 below. Each finding was checked against the source by its lane reviewer. + + +--- + +# Lane 1 + +phase: 33-seo-output-audit-and-content +lane: 1 of 4 (framework summercms v0.6.0..HEAD, sm-journal-plugin v0.3.1..HEAD) +reviewed: 2026-11-04T00:00:00Z +depth: standard +files_reviewed: 30 +files_reviewed_list: + - summercms.go/modules/lagoon/validate.go + - summercms.go/modules/lagoon/attach/variant.go + - summercms.go/modules/lagoon/attach/webp/webp.go + - summercms.go/modules/pact/content.go + - summercms.go/modules/pact/capabilities.go + - summercms.go/modules/cabana/field_type.go + - summercms.go/modules/cabana/http.go + - summercms.go/modules/cabana/schema_types.go + - summercms.go/modules/cabana/backend_users_admin.go + - summercms.go/modules/phrasebook/lang/en/validate.yaml + - summercms.go/modules/phrasebook/lang/pl/validate.yaml + - summercms.go/modules/phrasebook/lang/pl/validation.yaml + - summercms.go/scripts/check-phase33.sh + - sm-journal-plugin/classes/image_variants.go + - sm-journal-plugin/classes/headings.go + - sm-journal-plugin/classes/heading_fix.go + - sm-journal-plugin/classes/image_alt.go + - sm-journal-plugin/classes/related.go + - sm-journal-plugin/classes/journal_settings.go + - sm-journal-plugin/classes/content_facts.go + - sm-journal-plugin/classes/content_descriptors.go + - sm-journal-plugin/classes/menu_item_types.go + - sm-journal-plugin/classes/media_names.go + - sm-journal-plugin/classes/media_store.go + - sm-journal-plugin/console/image_variants.go + - sm-journal-plugin/console/database.go + - sm-journal-plugin/console/fix_headings.go + - sm-journal-plugin/console/import_posts.go + - sm-journal-plugin/controllers/api/settings.go + - sm-journal-plugin/controllers/api/maintenance.go + - sm-journal-plugin/controllers/api/posts.go + - sm-journal-plugin/controllers/api/posts_helpers.go + - sm-journal-plugin/models/settings.go + - sm-journal-plugin/models/image_variant.go + - sm-journal-plugin/updates/202610280001_add_golem15_journal_settings_headings.go + - sm-journal-plugin/updates/202610280002_create_golem15_journal_image_variants.go + - sm-journal-plugin/mcp.go + - sm-journal-plugin/jobs.go + - sm-journal-plugin/routes.go +findings: + critical: 2 + warning: 7 + info: 9 + total: 18 +status: issues_found +--- + +# Phase 33 (lane 1): Code Review Report + +**Reviewed:** 2026-11-04 +**Depth:** standard, with cross-file checks on the attach / webp / journal job chain and the H1 conversion call sites +**Status:** issues_found + +Paths are relative to `/media/nvme/dev/golem15/summercms.io/summercms/`. + +## Summary + +Framework (`v0.6.0..HEAD`, without `.planning` and `modules/boardwalk/dist`): the pact descriptor fields, `FormFieldType.RequestOptions` and `attach.VariantKey` are additive. `ContentDescriptor` is never serialised (`json:"-"` on `MenuItemType.Content`), so the new untagged `Kind` and `Tools` fields add no JSON keys. `mergeRequestOptions` copies maps and never changes the cached schema. The tightened `min`/`max` length rule in `lagoon.Validate` is a deliberate change and the v0.6.1 notes document it. I checked the consumers' `Rules()` maps: no rule that only fails now (a non-integer `min`/`max` bound) shows up, and the `max:5000`-style TEXT cases are the ones the notes call out. The gate script is careful: it uses `set -euo pipefail`, `${tmp:?}` guards and trapped scratch dirs. I found no defects in it. + +The main framework problem is a side effect of the new `attach/webp` package. Importing it registers gen2brain's WebP **decoder** ahead of `golang.org/x/image/webp`, so every binary that mounts Journal now decodes untrusted WebP uploads through native libwebp. I confirmed this by running it. + +Journal: two release-blocking defects. + +1. **Tall images.** For a tall original, the image variants job asks `ThumbKey` for a thumbnail taller than 4096 px. `ThumbKey` returns an error, and that error aborts the whole job and the `journal:image-variants` backfill on every run. +2. **Code samples in markdown.** The H1→H2 conversion (on by default) runs an HTML tokenizer over the **markdown** `content`. It rewrites `

` inside fenced and inline code samples. This happens on every API save, on import, and on `journal:fix-headings --apply`. I confirmed it by running it. + +There are also these warnings: +- `journal:fix-headings` has the same "database is not configured" CLI defect that was just fixed for `journal:image-variants`. +- The tokenizer rewrites drop trailing bytes. +- `images/alt` has a lost-update path and is silently reverted on markdown posts. +- Author pages publish admin logins. + +Documented decisions were not reported: +- `min`/`max` as a length; +- non-integer bounds now being an error; +- dynamic glibc linking; +- the default-on H1 setting; +- no foreign key on `image_variants.file_id`; +- the Polish `string` message (the file comments on it). + +## Critical Issues + +### CR-01: A tall original fails the image variants job and stops the whole backfill + +**File:** `sm-journal-plugin/classes/image_variants.go:189-201` (with `summercms.go/modules/lagoon/attach/thumb.go:29,169-171`) + +**Issue:** `sourceSize` only refuses originals over `attach.MaxImagePixels` (4096×4096 = 16.7 MP in area). The job then computes `h = sh*w/sw` and calls `f.ThumbKey(ctx, bucket, w, h, "exact")`. `ThumbKey` refuses any edge over `maxThumbEdge = 4096` with `attach: thumb size %dx%d is out of range`, and `GenerateImageVariants` returns that error at once (`return out, fmt.Errorf("journal: thumbnail %d at %d: %w", ...)`). + +**Evidence:** +- A 1000×9000 infographic is 9 MP, so it passes `sourceSize`. At w=480 the height is 4320, which is over 4096, so `ThumbKey` errors. +- The job then fails all 3 conga attempts. +- `journal:image-variants` walks files in id order and returns on the first error, so every featured image after the tall one never gets variants. +- The failure is permanent: every rerun stops at the same file, and every admin save of that post queues another failing job (`DispatchPostImageVariantsAfterCommit`). +- The v0.4.0 notes tell every site to run `journal:image-variants` once as the upgrade step. + +**Fix:** Treat a width whose height would leave the thumbnailer's range like a width wider than the original, and never let one file's thumbnail error abort the batch: +```go +h := max(1, int((int64(sh)*int64(w)+int64(sw)/2)/int64(sw))) +if h > maxVariantEdge { // 4096, the thumbnailer's edge limit + out.Skipped++ + continue +} +key, err := f.ThumbKey(ctx, bucket, w, h, ImageVariantsMode) +if err != nil { + if isStorageError(err) { return out, err } // retryable + variantsLog.Warn("journal: no image variant", "file", f.ID, "width", w, "error", err) + out.Skipped++ + continue +} +``` +Add a test with a 1000×9000 original that is followed by a normal one. + +### CR-02: The H1→H2 conversion rewrites `

` inside code samples of markdown posts (silent content corruption, on by default) + +**File:** +- `sm-journal-plugin/classes/headings.go:17-52, 68-75` +- call sites `controllers/api/posts_helpers.go:865` (Store/Update), `classes/heading_fix.go:155,180` (fix-headings), `console/import_posts.go:229,367` + +**Issue:** `ConvertContentH1(content, contentHTML)` calls `ConvertH1` on `content` as well as `content_html`. For a markdown post, `content` is markdown (the API derives `content_html` from it with `FormatHTML`, `posts_helpers.go:537`). The `x/net/html` tokenizer does not know markdown code spans or fences, so a code sample that shows `

` is rewritten to `

` in the stored markdown source. The count it reports comes from `content_html`, where goldmark has escaped the code, so the corruption is not even counted. The next render shows the edited sample to readers. The setting defaults to on (migration `DEFAULT TRUE`, `config.yaml content_h1_to_h2: true`), so every API or MCP save of a technical post that shows HTML corrupts it. `journal:fix-headings --apply` and `/fix-headings` do it in bulk across every post and translation. + +**Evidence:** I ran it with `GOWORK=off go run` against the plugin: +``` +in : "# Title\n\nExample:\n\n```html\n

Hello

\n```\n\nInline `

` tag." +content out: ```html\n

Hello

\n``` ... Inline `

` tag. +content_html:

Title

... <h1>Hello</h1> (correct) +n = 1 +``` + +**Fix:** Never run the HTML rewrite on markdown. Convert `content` only when it is HTML (the rich-editor case, `LooksLikeHTML(content)`). For markdown, leave the source alone and convert only the rendered `content_html`, which every render path re-derives and re-converts. If converting markdown ATX headings is wanted, do it on goldmark's AST, not with a tokenizer: +```go +func ConvertContentH1(content, contentHTML string) (string, string, int) { + contentHTML, nHTML := ConvertH1(contentHTML) + if LooksLikeHTML(content) { + var nContent int + content, nContent = ConvertH1(content) + if nHTML == 0 { nHTML = nContent } + } + return content, contentHTML, nHTML +} +``` +Add a regression test with a fenced `

` sample to `headings_test.go`. Re-check the translation path (`ConvertTranslationH1`) the same way. + +## Warnings + +### WR-01: `journal:fix-headings` cannot run from the application CLI (same defect as the one just fixed for image-variants) + +**File:** `sm-journal-plugin/console/fix_headings.go:50-53` + +**Issue:** The command uses `mustDB(app)` only and returns `journal: database is not configured` when no database is published. Commit `fc3c255` and `console/database.go:85-89` show that the generated CLI publishes no database for plugin commands. `journal:image-variants` was switched to `lookupDB` for this reason. `journal:fix-headings` was not, although the v0.4.0 notes give "run `journal:fix-headings` (dry run first)" as an upgrade step, and the summaries note that 33-13 needed an app wrapper for the image-variants case. + +**Fix:** Use the same helper: +```go +db, done, err := lookupDB(ctx, app) +if err != nil { return err } +defer done() +``` +Add a CLI test like `TestImageVariantsCommandCLI`. + +### WR-02: Importing `attach/webp` replaces the process-wide WebP decoder for all uploads with native libwebp + +**File:** `summercms.go/modules/lagoon/attach/webp/webp.go:17` (with `github.com/gen2brain/webp@v0.6.4/webp.go:252-257`) + +**Issue:** +- The package doc and the v0.6.1 notes say only that binaries importing `attach/webp` "link the encoder". +- gen2brain's `init` also calls `image.RegisterFormat("webp", "RIFF????WEBPVP8", ...)`. +- Since Go 1.21, unrelated packages are initialised in import-path order, so `github.com/gen2brain/webp` registers before `golang.org/x/image/webp` (imported by `attach/thumb.go:25`). +- `image.Decode` picks the first registered match. +- So in any binary that mounts Journal, `attach.ThumbKey`, `sourceSize` and every other `image.Decode` of **user-uploaded** WebP go through the system `libwebp.so` via purego, a memory-unsafe C decoder whose version the app does not control. The notes recommend installing it. + +**Evidence:** I decoded a corrupt WebP with only the framework package imported (`_ ".../attach/webp"`). The result was `format="webp" err=webp: decode failed`, which is gen2brain's `ErrDecode`. x/image would return a `vp8:` error. Decoding behaviour also changes: animated WebP now decodes its first frame, and errors read differently. + +**Fix:** Either document it in the package doc, the attachments docs and the v0.6.1 notes ("this package also takes over WebP decoding; keep libwebp patched, or build with `-tags nodynamic`"), or keep decoding pure Go by calling `encoder.Encode` only through a narrow wrapper package that does not register the format. That is not possible with gen2brain's `init`, so a decision note is needed. At minimum, `attach` should decode WebP explicitly with `xwebp.Decode` instead of `image.Decode`, so the encoder dependency cannot change the decoder. + +### WR-03: The tokenizer-based rewriters silently drop the trailing bytes of an unterminated tag + +**File:** `sm-journal-plugin/classes/headings.go:26-29`, `sm-journal-plugin/classes/image_alt.go:27-31` + +**Issue:** Both loops `break` on `xhtml.ErrorToken` without writing `z.Raw()`. When the input ends inside something the tokenizer reads as a tag (`a x

a x

a "`, with `` tag. A markdown image (`![old](src)`) is left with its old alt. Every API create or update re-derives `content_html` from `content` (`posts_helpers.go:537,593`), and so does the editor's markdown path (`blog_editor.go:336`). So the next content edit silently restores the missing or old alt that the SEO audit flagged, and the MCP tool reported `changed: true`. + +**Fix:** For markdown content, rewrite the matching `![alt](src)` image (goldmark AST, or a markdown-aware rewrite). Otherwise refuse with 422 ("post content is markdown; edit the post") instead of answering success. + +### WR-06: Author pages publish backend administrators' login names + +**File:** `sm-journal-plugin/classes/content_facts.go:180-194`, `sm-journal-plugin/classes/menu_item_types.go:59, 847+` + +**Issue:** `authorRow.slug()` falls back to `login` and `name()` falls back to `login`. The new `journal-author` content descriptor and menu type (`author_pages` defaults to true) feed sitemaps, breadcrumbs and the JSON-LD `Person` node. As a result, every backend user with a published post and no `golem15_bloghub_author_slug` gets their admin login published at `/blog/author/`, in the sitemap and in structured data. The author API already accepted login lookups. The new part is that these URLs are now listed and crawled automatically. Admin logins are half of the backend credential pair. + +**Fix:** Skip authors without an author slug in `listAuthors`, `resolveAuthor` and post `Authors[].Ref` (still match by login for old links). Alternatively, generate a slug from the first and last name. Never use `login` as the display name in the descriptor. State the behaviour in the v0.4.0 notes. + +### WR-07: The settings API keeps the stale `string`/`url` workaround, which now disagrees with the admin + +**File:** `sm-journal-plugin/controllers/api/settings.go:188-211` (comment at 188-190) + +**Issue:** The comment says `lagoon.Validate` "does not know" `string`/`url`. Since v0.6.1 it does, and the plugin requires v0.6.1. The workaround's url check (`http`/`https` with a host) is stricter than the framework's Laravel `urlPattern`, which accepts `ftp://`, `git://` and others. The same `rss_image_url` value can therefore be saved in Settings › Journal but gets a 422 over `PUT /settings` and MCP. A `string` value with a number in it is also reported twice once the type check and Fill both run. + +**Fix:** Drop `settingsRule` and pass the full rules to `lagoon.Validate`. If https-only is really wanted for the RSS image, express it in `Rules()` so both paths share it. + +## Info + +### IN-01: Images narrower than every width are re-queued and re-listed forever + +**File:** `sm-journal-plugin/classes/image_variants.go:189-193, 227-236` + +An original narrower than 480 px (or one skipped as unusable) never gets a row, so the `NOT EXISTS` branch of `featuredFiles(missing)` matches it on every run. Each admin save of the post queues a job that reads the original and does nothing. The backfill never prints "Every featured image has its variants." Fix: record a "no variants" marker (for example a row with `width = 0` and the source size), or have `featuredFiles` compare against the source size stored in that row. + +### IN-02: Concurrent variant jobs can delete each other's WebP after it has been recorded + +**File:** `summercms.go/modules/lagoon/attach/webp/webp.go:63-104` + +`Variant` checks `Exists`, writes, and on a write or close failure deletes `key`. If two jobs for the same file overlap, for example the upload dispatch and an admin-save dispatch, one job's failed write can delete the variant the other just wrote and inserted a row for. `` then gets a 404 and browsers do not fall back to ``. Fix: write to a temporary key and rename or copy it into place, or delete only when this call created the object (check `Exists` again before deleting). Alternatively, take a per-file advisory lock in the job (`pg_try_advisory_xact_lock(file_id)`). + +### IN-03: `DeleteFeaturedImage` racing a running job leaves orphan blobs and rows + +**File:** `sm-journal-plugin/controllers/api/posts.go:538-547` + +The prefix delete runs while a job that has already loaded the file row can still write thumbnails, WebP copies and a variant row afterwards. Orphans are never listed but take storage. Consider checking that the file row still exists before the insert in `GenerateImageVariants`. + +### IN-04: The `featuredFiles` OR clause is parenthesised only by chance + +**File:** `sm-journal-plugin/classes/image_variants.go:232-235` + +GORM wraps a raw `Where` in parentheses only when the upper-cased SQL contains `" AND "` or `" OR "` with spaces on both sides. The top-level `OR` here comes after a newline, so it does not trigger wrapping. Only the inner `system_files.id AND v.width` does. Reformatting the string could turn it into `a AND b AND NOT EXISTS ... OR EXISTS ...` and list other posts' files. Wrap the expression in explicit `( ... )`. + +### IN-05: `lagoon.Validate` stops at the first failing group, so later messages are missing + +**File:** `summercms.go/modules/lagoon/validate.go:174-199` + +A `url`/`string`/length failure returns before the `email`/`in` tags and `unique` run. For example, `nullable|email|max:255` with an over-long invalid address reports only `max`, where Laravel reports both. This is not a correctness hole, but it differs from the PHP contract. + +### IN-06: `between` measures bytes while `min`/`max` measure runes + +**File:** `summercms.go/modules/lagoon/validate.go:218-230` (`n = len(s)` at 221) vs `ruleLength` at 409-440 + +`between:3,5` on `"żółw"` counts 7, while `min:3|max:5` counts 4. Laravel uses `mb_strlen` for both. Use `ruleLength` in the `between` branch too. + +### IN-07: The release note's "Settings forms that use string or url can be saved again" is not true for every core form + +**File:** `summercms.go/.planning/phases/33-seo-output-audit-and-content/33-14-notes-summercms-v0.6.1.md` ("Fixed") + +`sm-paymentgateway-plugin/models/currency.go:43` and `order.go:125` use `required|string|size:3`, and `size` is still an unrecognised rule in `lagoon.Validate`. Those admin saves still answer 500 (the error moved from `string` to `size`). Either add `size` or narrow the note. + +### IN-08: The `fix_headings` MCP tool is not marked destructive + +**File:** `sm-journal-plugin/mcp.go:219-228` + +With `dry_run: false` it rewrites every post and category body, yet unlike `set_image_alt`/`delete_post` it has no `Destructive: &destructive`. MCP clients will not ask for confirmation. + +### IN-09: The Polish `string` message now differs from the PHP backend + +**File:** `summercms.go/modules/phrasebook/lang/pl/validation.yaml:63-66` + +`lagoon::validation.string` is also used by `ValidateRequest` on ported endpoints, so a `pl` request now gets a Polish line where the PHP backend answers in English. The file comments on this, so it is deliberate, but it conflicts with the "response shapes are not improved during a port" rule in CLAUDE.md for any client that matches on message text. Confirm the choice for ported apps. + +--- + +_Reviewed: 2026-11-04_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_ + +--- + +# Lane 2 + +phase: 33-seo-output-audit-and-content +lane: 2 of 4 (sm-sitemap-plugin, git diff v0.1.2..HEAD, dist and lock files excluded) +reviewed: 2026-11-04T00:00:00Z +depth: standard +files_reviewed: 38 +files_reviewed_list: + - sm-sitemap-plugin/classes/rebuild.go + - sm-sitemap-plugin/classes/slots.go + - sm-sitemap-plugin/classes/writer.go + - sm-sitemap-plugin/classes/settings.go + - sm-sitemap-plugin/classes/lastmod.go + - sm-sitemap-plugin/classes/dispatch.go + - sm-sitemap-plugin/classes/images.go + - sm-sitemap-plugin/classes/types.go + - sm-sitemap-plugin/classes/typesettings.go + - sm-sitemap-plugin/classes/files.go + - sm-sitemap-plugin/classes/news.go + - sm-sitemap-plugin/classes/indexnow.go + - sm-sitemap-plugin/classes/listing.go + - sm-sitemap-plugin/classes/htmlgroups.go + - sm-sitemap-plugin/contract/contract.go + - sm-sitemap-plugin/controllers/settings.go + - sm-sitemap-plugin/controllers/settings/_sitemap_files.htm + - sm-sitemap-plugin/controllers/settings/_indexnow_status.htm + - sm-sitemap-plugin/controllers/api/auth.go + - sm-sitemap-plugin/controllers/api/settings.go + - sm-sitemap-plugin/controllers/api/html.go + - sm-sitemap-plugin/controllers/api/indexnow.go + - sm-sitemap-plugin/models/settings.go + - sm-sitemap-plugin/models/settings/fields.yaml + - sm-sitemap-plugin/models/url.go + - sm-sitemap-plugin/models/legacy.go + - sm-sitemap-plugin/updates/202610280001_create_golem15_sitemap_urls.go + - sm-sitemap-plugin/updates/202610280002_update_golem15_sitemap_settings.go + - sm-sitemap-plugin/updates/202610280003_create_golem15_sitemap_types.go + - sm-sitemap-plugin/updates/202610280004_add_golem15_sitemap_media_news_indexnow.go + - sm-sitemap-plugin/console/rebuild.go + - sm-sitemap-plugin/console/import.go + - sm-sitemap-plugin/console/import_winter.go + - sm-sitemap-plugin/console/helpers.go + - sm-sitemap-plugin/routes.go + - sm-sitemap-plugin/jobs.go + - sm-sitemap-plugin/listeners.go + - sm-sitemap-plugin/extensions_settings.go + - sm-sitemap-plugin/mcp.go + - sm-sitemap-plugin/plugin.go + - sm-sitemap-plugin/admin.go + - sm-sitemap-plugin/config/config.yaml +findings: + critical: 0 + warning: 8 + info: 11 + total: 19 +status: issues_found +--- + +# Phase 33, lane 2: sm-sitemap-plugin Code Review Report + +**Reviewed:** 2026-11-04 +**Depth:** standard, with cross-file checks against cabana, conga, fetchguard, signpost, pact and party (summercms.go), and against v0.1.2 for the migration +**Files Reviewed:** 38 source files (the list above also includes 4 config and template files read for context) +**Status:** issues_found + +All paths below are relative to `/media/nvme/dev/golem15/summercms.io/summercms/sm-sitemap-plugin/`. + +## Summary + +Several areas held up under adversarial reading: + +- **XML escaping.** Every text and attribute value goes through `xml.EscapeText`. +- **SQL.** Every query is parameterized. The only concatenation is constant column DDL. +- **IndexNow.** Requests are sent to one fixed host only: fetchguard `AllowHostsMode`, HTTPS, 10 s timeout, and the site's own addresses are filtered on the site host. I found no SSRF. The key is public by design (T-33-14). +- **Management API and MCP tools.** Every route goes through `cabana.ResolveBackend` with the token cover `golem15.sitemap.*`, then `cabana.Allows(PermissionManage)`. The MCP dispatch marker comes from context and cannot be forged through a header. I found no authz gap. +- **Admin partials.** They render through `html/template`, so a Filter's free-text reason cannot inject markup. + +The real problems are in the data flow between the record-event listeners, the rebuild and IndexNow, in how rebuilds run concurrently, and in the migration: + +- Deleted records never reach IndexNow. Content edits never do either. +- Two rebuild jobs can run at once. Only the write phase is serialized, so an older snapshot can overwrite a newer one. +- A rebuild can silently revert a lastmod that a concurrent record save just moved. +- The definitions migration silently drops url items that v0.1.2 served: relative paths, paths with spaces, and absolute URLs with `&`. +- The first save of the settings form pins every HTML sitemap heading to the admin's UI language. + +Sitemap size limits are only half enforced. The 50,000 URL count is enforced. The 50 MB byte limit is not, and image files and many-locale hreflang files can exceed it. + +I found nothing that I would classify as critical (a security hole or unrecoverable data loss). The migration loss is recoverable, because the definitions table is kept. Several of the warnings still change what search engines see, and the release notes contradict WR-01 and WR-04. I recommend fixing WR-01 to WR-05 before v0.2.0. + +## Warnings + +### WR-01: IndexNow never hears of deleted records, and never of content edits + +**File:** `classes/lastmod.go:140-156`, `classes/rebuild.go:488-506`, `classes/indexnow.go:135-136`. Test gap: `sitemap_extras_test.go:436-442`. + +**Issue:** `RecordDeleted` sets the record's rows to `excluded_reason = 'deleted'` directly, as soon as the delete commits. The rebuild that runs next finds those rows unmatched and computes `reason = deleted`. It then hits `if r.ExcludedReason == reason { continue }` (rebuild.go:498) before `res.Removed` is appended at line 501. So an address removed by a real delete is never in `RebuildResult.Removed`, and `NotifyIndexNow` never sends it. + +In the same way, `RecordSaved` moves `lastmod` directly in the table (lastmod.go:131). `apply` fills `res.Changed` only when the URL string changed (rebuild.go:481). So an edited article that keeps its URL never pings IndexNow. + +The admin framework fires `RecordDeleted` for every admin delete (pact/record_events.go). In production this is the normal path for removing content. + +This contradicts: +- the settings copy ("notify IndexNow engines of new, changed and removed addresses"); +- 33-04's truth "posts the added, changed and removed URLs"; +- the v0.2.0 release notes ("pings of added, changed and removed addresses"). + +**Evidence:** `TestIndexNow` removes a record with `contentStore.remove(postType, 1)` (content_fixture_test.go:79). That only edits the fake store and fires no `RecordDeleted`. The test therefore covers only the path that production never takes. + +**Fix:** Have the listeners record what they changed, so the rebuild or IndexNow can see it. Either option works: +- (a) In `RecordDeleted`, collect the absolute URLs of rows that were listed (`excluded_reason = ''`) before the update and enqueue `IndexNowArgs` for them (production and an active key only). In `RecordSaved`, do the same for the rows whose lastmod moved. +- (b) Add a `notified_at`/`indexnow_pending` flag that the listeners set and `apply` reads into `res.Removed` and `res.Changed`. + +Add a test that fires `pact.RecordDeleted` and a content-only `pact.RecordSaved`, then asserts the queued IndexNow job. + +### WR-02: Concurrent rebuilds: an older snapshot can overwrite a newer one + +**File:** `classes/rebuild.go:133-158` (collect outside the lock), `jobs.go:339`, `classes/dispatch.go:252-266`, `classes/lastmod.go:184-199`, `schedule.go:396`, `controllers/api/settings.go:337-350`. + +**Issue:** `Rebuild` reads the settings, the type settings and every content record (`collect`) before it takes `SELECT ... FOR UPDATE` on the settings row. Only `apply` is serialized. The doc comment says "two rebuilds never interleave", but that is true only for the write phase. + +Several things can start a rebuild, and nothing makes the job unique: +- a settings save; +- `POST /_sitemap/api/v1/rebuild` or the `sitemap_rebuild` MCP tool (300 a minute); +- the debounced job; +- the nightly in-process `sitemap:rebuild`; +- jobs that piled up while no worker was running. + +conga's default queue runs 4 workers, so two rebuilds really can run concurrently. + +Take two rebuilds, A and B: +1. A collects. +2. Record X is published. +3. B collects, applies and commits. X is listed and queued for IndexNow as added. +4. A then applies its older snapshot. X is unmatched and marked `unpublished` (A saw it unpublished). It drops out of the files and is sent to IndexNow as removed. + +Nothing fixes this until another record event or the nightly run. A's stale `settings` also writes stale additional addresses. + +**Fix:** Take the lock before `collect`, for example `pg_advisory_xact_lock(hashtext('golem15.sitemap.rebuild'))` in a transaction that wraps both `collect` and `apply`, or a session advisory lock held for the whole `Rebuild`. Either way, the second rebuild collects after the first has committed. You can additionally insert the job with River unique options (ByArgs plus ByState available/scheduled), so bursts collapse to a single rebuild. + +### WR-03: A rebuild can revert a lastmod that a record save just moved + +**File:** `classes/rebuild.go:415` (snapshot), `:559-573` (`"lastmod": r.LastMod` written for every dirty row); `classes/lastmod.go:123-133`. + +**Issue:** `apply` reads the whole slot table at the start of its transaction (READ COMMITTED). For each dirty row it later writes back all columns, including `lastmod` with the value it read. `RecordSaved` runs after the record's commit and updates `lastmod` outside any sitemap lock. + +The race goes like this: +1. A save changes a title. +2. A running rebuild collects the new title and reads the old lastmod. The row is dirty because the title changed. +3. `RecordSaved` commits `lastmod = now`. +4. The rebuild's UPDATE writes the old lastmod back. + +A rebuild never moves lastmod for content rows (rebuild.go:472-474 only covers extras), so the lost update is permanent. The file then advertises a stale lastmod for an edited page, which is exactly what D-30 ("real lastmod") is meant to prevent. + +**Fix:** Do not write `lastmod` for content rows in `apply`. Include it in the `Updates` map only when `w.extra` is set, or for newly created rows. Alternatively, have `RecordSaved` take the same rebuild lock as WR-02. + +### WR-04: The definitions migration silently drops url items that v0.1.2 served + +**File:** `updates/202610280002_update_golem15_sitemap_settings.go:126-143`, `models/url.go:422-432`, `models/settings.go:237-257`. + +**Issue:** `copyDefinitions` keeps only items that pass `models.AllowedAddress`. That check is much stricter than the Phase 17 rule that accepted and served these items: `classes.AllowedItemURL` in v0.1.2 urlto.go:545, rendered through `URLTo`. The migration skips each of the following silently, with no log line and nothing in the release notes: +- relative paths without a leading slash (`o-nas`, `blog`): v0.1.2 accepted them and `URLTo` resolved them to `root/o-nas`; +- paths with whitespace (`/blog/zażółć gęślą`): v0.1.2 percent-encoded them; +- paths containing `"`, `<`, `>` or `\`; +- every absolute URL containing `&` (for example `https://site/x?a=1&b=2`), because `AllowedFrontendURL` refuses `&`; +- absolute URLs with whitespace. + +If `ParseLegacyItems` fails on one malformed item, the whole definition is dropped (lines 128-133), again without a word. The definitions table is kept, so the data can be recovered by hand. But nobody is told to do that, and the release note says "the definition's url items move to the additional addresses". + +The same rule means the new settings form and `sitemap:import` can never hold an absolute URL with a query string. A path with `&` is accepted, however (`AllowedAddress` does not refuse `&` for paths), which is inconsistent. + +**Fix:** In the migration, normalize before checking: +- `p` without a scheme or a leading `/` becomes `"/" + p`; +- whitespace and other unsafe bytes are percent-encoded (`url.PathEscape` per segment); +- `&` is allowed in absolute URLs (escaping already happens at write time, writer.go:70-72). + +For anything still dropped, log one line per item through `pluginLog.Warn` (the item index and the reason), and mention it in the release notes ("check the startup log for dropped addresses"). Parse items one by one, so a single bad item does not drop the rest. Relax `AllowedFrontendURL` for extras, or use a separate `AllowedExtraURL` that permits `&`. + +### WR-05: The first settings save pins every HTML sitemap heading to the admin's UI language + +**File:** `extensions_settings.go:129-133`, `classes/typesettings.go:76-78`, `classes/htmlgroups.go:158-163, 244-248`. + +**Issue:** When a type has no saved title, the form extension's `Load` fills the mltext with `{: Phrase(ctx, tr, typ.Label)}`. `ctx` carries the administrator's request locale, not the site default locale. Any save of the settings form, on any tab, sends this value back, and `SaveTypeSettings` stores it as the type's `titles`. + +From then on: +- `pickTitle` returns that single string for every locale without its own title, so the label's per-locale translation (`translateIn`) is never used again; +- it also overrides the Filter's localized group titles (htmlgroups.go:246-248 replaces `titles[cfg.Slug]`). + +Example: a pl/en site edited by an admin whose UI is in English. After one save, the Polish HTML sitemap shows "Posts", where it showed "Wpisy" before. + +**Fix:** Leave the mltext empty in `Load` when nothing is saved, and show the label as a placeholder or comment instead. Or have `SaveTypeSettings` skip storing titles that equal the translated label. If you keep a prefill, translate in the target locale (`towel.WithLocale(ctx, def)`) and fill every configured locale, not only `def`. + +### WR-06: A default file slug can collide with a reserved slug or with another type's slug + +**File:** `classes/types.go:365-374`, `classes/rebuild.go:52-73`, `routes.go:168`, `classes/files.go:31-47`, `classes/typesettings.go:65-70`. + +**Issue:** `EffectiveType` uses `DefaultSlug(typ.Key)` without checking `ReservedSlugs` or uniqueness. Only `SaveTypeSettings` and `decide` (for Filter groups) check them. Two cases follow: + +- A content type keyed `*-image` or `*-images` gets group `images`. Its files `sitemap-images-.xml`: + - appear in the index twice: once from the per-group query and once from the image query (files.go:31-47); + - are always routed to `imageFile` (routes.go:168), so its addresses are never served. +- Two types with the same last key segment share a group (for example `journal-category` and `shop-category`, both `categories`). Both are numbered into the same files, and the HTML sitemap merges them under the first type's label. + +In both cases `SaveTypeSettings` refuses any save of the Content types tab, including from the API or MCP when `types` is sent. The refusal goes on a field the admin did not touch, until the admin renames the slug by hand. Until then the rebuild keeps the collision. + +**Fix:** In `EffectiveType` (or once at boot), resolve default slugs deterministically. For a reserved slug append a suffix (`images` becomes `images-`). For later duplicates, use the type key with non-slug characters mapped to `-` (`shop-category`). Log the fallback. Use the same resolved slugs in `SaveTypeSettings`, so a save never fails on a type nobody edited. + +### WR-07: No 50 MB guard: image and hreflang files can exceed the protocol limit + +**File:** `classes/writer.go:97-158`, `classes/rebuild.go:542-547`, `classes/images.go:287`, `models/settings.go:66-69`. + +**Issue:** File size is counted only in URLs (1..50,000). The bytes are never measured. + +- **Image files.** These use the same file size as the URL files and allow up to 1000 `` entries per page, at about 80 bytes each. At the default `file_size` of 1000, 1000 gallery pages with 100 images each make about 8 MB. At `file_size` 50,000 with 15 images per page, a file is about 60 MB. +- **URL files with hreflang.** Each alternate adds about 90 bytes. 50,000 URLs with 10 locales plus x-default come to roughly 55 MB. + +Google rejects a file over 50 MB uncompressed, so the whole file (and its addresses) is lost. + +**Fix:** Cap the image files separately, for example by counting images instead of pages when numbering them (`AssignSlots` keyed on image count), or with a fixed smaller page count. Alternatively, have `WriteURLSet` and `WriteImageURLSet` stop at about 49 MB and log a warning. At minimum, clamp the effective `file_size` for image files, and for rows with alternates, so that worst-case bytes × count stays under 50 MB. Document the limit next to `file_size`. + +### WR-08: Additional addresses can point at any host and still go into the sitemap + +**File:** `classes/rebuild.go:269-286`, `classes/settings.go:314-325`, `models/url.go:422-432`. + +**Issue:** `AllowedAddress` accepts any absolute http(s) URL, and `Absolute` keeps it as it is. So an additional address `https://other-site.example/x` lands in `sitemap-pages-.xml`. The model comment says "an absolute http(s) URL on the site", but nothing enforces it. + +The sitemaps protocol allows only URLs on the sitemap's own host. Google reports such entries as errors and can distrust the file. IndexNow filters them out (`SiteURLs`), which shows the same rule was intended. The input is admin-only, so this is a correctness problem, not a privilege escalation. The same applies to API and MCP writes (`update_settings` `extra_urls`). + +**Fix:** In `Settings.Problems`, refuse an absolute extra URL whose host differs from `SiteURL(app, s)`. That needs the app, or a host passed in, as in `classes.SiteURLs`. Also, in `extraRows`/`Absolute` at rebuild time, drop absolute URLs on another host, with a warning. + +## Info + +### IN-01: `RecordSaved` dereferences `ev` before its nil check + +**File:** `classes/lastmod.go:113-115`. `contentTypeOf(app, ev.Model)` runs before `|| ev == nil`, so a nil event panics in the listener. `RecordDeleted` does the check in the right order. Swap the order. + +### IN-02: A URL change notifies only the new address + +**File:** `classes/rebuild.go:481-482`. `res.Changed` gets the new URL, but the old one is never sent, so engines are not prompted to recrawl the old address and see its 404 or redirect. Append `Absolute(site, before.URL)` too, for example to `Removed`. + +### IN-03: Non-ASCII and unsafe characters are written raw into `` + +**File:** `classes/settings.go:314-325`, `classes/writer.go:109-111`. v0.1.2 percent-encoded paths (`BuildURL`). Now `/ścieżka` reaches `` unencoded. The protocol requires RFC 3986 escaping. Google tolerates the raw form, but other consumers may not. Percent-encode non-ASCII bytes in `Absolute`. + +### IN-04: An index or file lastmod does not move when an address leaves a file + +**File:** `classes/files.go:31-33`. `MAX(lastmod)` covers listed rows only, so a deletion or unpublish that empties a slot leaves the file's index lastmod unchanged. Crawlers that rely on it may keep the removed URL. Consider `GREATEST(MAX(lastmod), MAX(updated_at) FILTER (WHERE excluded_reason <> ''))` per file. + +### IN-05: `rebuild_delay_seconds: 0` disables debouncing completely + +**File:** `classes/lastmod.go:64-72, 191-193`. With delay 0, `rebuild_pending_until = NOW()` is already `<= NOW()` for the next event, so every record event enqueues a full rebuild. Clamp it to at least 1 s, or document the effect. + +### IN-06: Records paging guards only against a cursor that repeats itself + +**File:** `classes/rebuild.go:252-255`. `page.Next == after` stops a self-loop, but a content type whose cursors cycle (A, B, A) loops forever inside the rebuild job. Cap the number of pages, or track the cursors already seen. + +### IN-07: The "building" state stays on for an hour after a failed rebuild + +**File:** `classes/dispatch.go:270-275`. `built_at` is set only on success, so a Filter error or a database error leaves S1 and `get_settings` saying "building" until `buildingWindow` passes. Record a `rebuild_failed_at`, or clear `rebuild_requested_at` when the job fails for the last time. + +### IN-08: The slot table only grows + +**File:** `classes/rebuild.go:277-283, 488-506`. Rows are never deleted. Every edit of an additional address's URL creates a new row (`ExtraRecordID` is the URL's hash) and leaves the old one as a permanent `deleted` hole. Content churn leaves holes that are never refilled, so files get sparser and every rebuild loads all tombstones. Consider pruning `deleted` rows older than N days during a renumber, or compacting files whose holes exceed a ratio. + +### IN-09: The sitemap index can pass 50,000 entries + +**File:** `classes/files.go:20-63`, `models/settings.go:141`. With a small `file_size` (as low as 1) and many addresses, the index is never split. The default of 1000 makes this theoretical, but there is no check. Either refuse `file_size` values that would produce more than 50,000 files, or emit nested indexes. + +### IN-10: The duplicate-extras check compares addresses only loosely + +**File:** `classes/rebuild.go:297-319`. `addressKey` does not lowercase the host or normalize a trailing slash, so `https://EXAMPLE.com/blog/a` or `/blog/a/` still duplicates the record's `/blog/a`. Two extras that name the same page in different forms (`/a` and `https://site/a`) get different `ExtraRecordID`s and are both listed. Normalize (lowercase the scheme and host, drop the default port, and apply one trailing-slash rule) before comparing and before hashing. + +### IN-11: Smaller points + +- `models/settings.go:317-333`: `ParseDate` reads naive datetimes as UTC. Dates exported by WinterCMS are in the app timezone, so imported lastmods shift by 1 to 2 h. Use `app.timezone` for naive layouts. +- `controllers/api/auth.go:22`: a single token cover, `golem15.sitemap.*`, serves both GET and PUT, so a read-only token cannot be issued. Consider `golem15.sitemap.read` for GET. +- `controllers/settings.go:101-104`: `FormAfterUpdate` silently skips the IndexNow key and the rebuild when the database lookup fails. Log it. +- `controllers/api/html.go` plus `classes/htmlgroups.go:388-399`: the public HTML API reads up to 10,000 rows per tree group per request at 300 a minute per IP. Consider lowering `sitemap-html`, or caching the tree per rebuild (`built_at`). + +--- + +_Reviewed: 2026-11-04_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_ + +--- + +# Lane 3 + +phase: 33-seo-output-audit-and-content +lane: 3 of 4 (sm-seo-plugin, git diff v0.1.1..HEAD at 075cf17; assets/dist and lock files excluded) +reviewed: 2026-11-04T20:20:54Z +depth: standard (deep on classes/crawl, render_chrome.go, auto-links) +files_reviewed: 46 +files_reviewed_list: + - classes/crawl/crawler.go + - classes/crawl/fetch.go + - classes/crawl/render.go + - classes/crawl/render_chrome.go + - classes/crawl/job.go + - classes/crawl/store.go + - classes/crawl/actions.go + - classes/crawl/checks.go + - classes/crawl/links.go + - classes/crawl/orphans.go + - classes/crawl/compare.go + - classes/crawl/parse.go + - classes/crawl/fix.go + - classes/autolinks.go + - classes/linkgraph.go + - classes/pagination.go + - classes/resolver.go + - classes/breadcrumbs.go + - classes/registry.go + - classes/llms.go + - classes/site.go + - classes/structured.go + - classes/sitemap_filter.go + - classes/schema/custom.go + - controllers/api/crawls.go + - controllers/api/issues.go + - controllers/api/registry.go + - controllers/api/graph.go + - controllers/api/links.go + - controllers/api/auth.go + - controllers/admin_registry.go + - controllers/autolinks.go + - controllers/crawls.go + - controllers/issues/_issue_details.htm + - controllers/crawls/_crawl_summary.htm + - controllers/settings/_publisher_preview.htm + - models/autolink.go + - models/settings.go + - routes.go + - mcp.go + - jobs.go + - commands.go + - schedule.go + - listeners.go + - assets/src/elements/links.ts + - assets/src/elements/graph.ts + - updates/202610280002_create_golem15_seo_pages.go + - updates/202610280003_add_golem15_seo_links_and_llms.go + - updates/202610280004_add_golem15_seo_publisher_pagination.go + - updates/202610280005_create_golem15_seo_crawls.go + - updates/202610280006_add_golem15_seo_audit_settings.go + - updates/202610280007_create_golem15_seo_autolinks.go +findings: + critical: 1 + warning: 6 + info: 7 + total: 14 +status: issues_found +--- + +# Phase 33 lane 3 (sm-seo-plugin): Code Review Report + +**Reviewed:** 2026-11-04T20:20:54Z +**Depth:** standard, deep on the crawler, the chromedp renderer and auto-links +**Status:** issues_found + +## Summary + +I compared the diff against the 33-05, 33-06, 33-10 and 33-11 SUMMARYs and the chromedp decision note. The resolver catch-all-last order matches the note, so it is not a finding. + +Checked and clean: +- **Crawler SSRF.** fetchguard AllowHostsMode checks addresses at dial time and follows no redirects. `Fetcher.Allowed` and `Fetcher.Key` keep the crawler on the site host and its www twin. +- **Chrome SSRF.** The host is resolved and checked with `IsPrivateAddr`, pinned to one address per host, and the resolver rules end with `MAP * ~NOTFOUND`. Requests are intercepted, a fresh profile is used per crawl, and `Close` is deferred. +- **Open redirects.** Pagination 301s are always `origin + path`, and `origin` is the configured site or its twin. +- **Auth.** Every new management route goes through `manage()` and requires `golem15.seo.manage`. Every MCP read tool is a GET. The route-report and notfound tokens use constant-time comparison. +- **Auto-link URLs.** `ValidAutolinkURL` refuses `javascript:`, `data:`, protocol-relative and `/\` addresses. The layer escapes the href and the inserted phrase. +- **Migrations 0002 to 0007** only add tables and columns, so no data is lost. + +One blocker remains: a crawl that is cancelled while running, or that reaches the job's 6 h timeout, stores nothing. In the timeout case the crawl stays "running" for good, which blocks every later crawl, including the nightly one. + +## Critical Issues + +### CR-01: A crawl cancelled or timed out while running loses its results and can stay "running" forever + +**File:** `classes/crawl/job.go:156-179`, `classes/crawl/job.go:241-255`, `classes/crawl/job.go:275-288`, `jobs.go:19`, `classes/crawl/job.go:397-416` + +**Issue:** `Run` builds `db := gdb.WithContext(ctx)` on the River job context, then calls `finish(ctx, svc, db, ...)` with that same context. That context is cancelled in two cases: + +- **User cancel.** `Cancel` calls `conga.CancelJob`, which calls `river.Client.JobCancel`. River cancels the context of a running job; its docs say so, and River's `CancelRunningJobPollOnly` test shows it also happens in poll-only mode. +- **Timeout.** `conga.Timeout(6*time.Hour)` in `jobs.go:19` cancels it when the deadline passes. + +In both cases `Crawl` stops and returns `res, nil`. `finish` then runs every query on the dead context: + +- `db.Raw(...).Scan(¤t)` fails, and the error is ignored. +- `LoadLimits(db)` fails, and `finish` returns at job.go:252-255 before the store transaction, the status update, `FailJob`/`StopJob` and `Prune`. + +The consequences: + +- **Cancel of a running crawl.** + - The row stays at the "cancelled" status `Cancel` wrote, with `finished_at` NULL and no pages, links or issues stored. That contradicts the MCP tool description ("the pages fetched so far and their issues are kept", mcp.go `cancel_crawl`) and the `CancelCrawl` doc comment. + - `LatestCrawl` (store.go:169) returns this empty crawl, so `CurrentScope` turns the Site audit and `GET /issues` empty and hides every earlier finding. + - `Duration` keeps growing because `finished_at` is never set. +- **Timeout.** + - The row stays `running` with nothing stored. + - `ActiveCrawl` keeps returning it, so every `Start` answers 409 `crawls.already_running`, and `seo:crawl --scheduled` logs "still running" every night (commands.go) until an admin cancels it by hand. + - The settings make this reachable on purpose: `crawl_max_pages` up to 50 000 and `crawl_delay` up to 5000 ms give about 69 h of pacing alone (see WR-02). +- **Worker restart.** A restart mid-crawl (`MaxAttempts(1)`) leaves the same stuck "running" row; nothing ever recovers a stale crawl. + +**Evidence:** +- job.go:161 `db := gdb.WithContext(ctx)`; job.go:179 `return finish(ctx, svc, db, row, res, runErr)`. +- job.go:252-255: `limits, err := LoadLimits(db); if err != nil { return err }`. +- crawler.go:219-223: on `ctx.Err()` the crawl sets `Canceled` and returns `res, nil`. +- River v0.47.0 client.go:1428-1434 documents that the context of a running job is cancelled. +- The only cancel test (crawl_admin_test.go:205-221) cancels a queued crawl, so this path is not tested. + +**Fix:** Write the final state on a context that the job's cancellation cannot reach, with its own deadline, and let the stale-crawl case recover: + +```go +func Run(ctx context.Context, svc *classes.Service, id uint) error { + ... + res, runErr := run(ctx, svc, db, row) + // The job context is cancelled by a cancel or the job timeout; storing + // what was fetched must not depend on it. + fctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Minute) + defer cancel() + return finish(fctx, svc, gdb.WithContext(fctx), row, res, runErr) +} +``` + +Also: +- In `finish`, fall back to the defaults when `LoadLimits` fails instead of returning. +- Map `ctx.Err() == context.DeadlineExceeded` to `failed` with a "time limit" note. +- In `Start`/`ActiveCrawl`, treat a `running` row whose conga or River job is no longer running, or whose `started_at` is older than the job timeout, as failed. +- Pages whose fetch was cut short by the cancellation currently get `Err` "network_error" (crawler.go:249) and would be stored as `page_status` errors once the store works. Map the error to "canceled" when `ctx.Err() != nil`. + +## Warnings + +### WR-01: Cancelling a running crawl clears the one-crawl-at-a-time guard while its job still runs + +**File:** `classes/crawl/job.go:397-416`, `classes/crawl/store.go:181-190`, `classes/crawl/actions.go:251-274` + +**Issue:** `Cancel` sets `status = cancelled` as soon as the request runs, but the job stops later: one batch of up to two fetches (10 s each), plus a render (up to 20 s), plus `finish`. During that window: + +- `ActiveCrawl` ignores `cancelled` rows, so a new `Start` succeeds. Two crawls then run against the site at once, possibly with two Chrome processes, which breaks the "one crawl at a time" guarantee (P33-39). +- `DeleteCrawls` refuses only `queued`/`running` rows and `Prune` keeps only `NOT IN ('queued','running')`, so the still-running crawl can be deleted. Its later `SavePages` then hits the `crawl_id` foreign key and the store fails. + +**Evidence:** job.go:402 writes `status: cancelled` immediately; store.go:183 `status IN (queued, running)`; actions.go:256 counts only queued/running. + +**Fix:** Let the job own the terminal status. For a running crawl, have `Cancel` set a `cancel_requested` flag, or keep `running` and set `cancelled_by`. Have `finish` write `cancelled`. Have `ActiveCrawl`, `DeleteCrawls` and `Prune` treat a row with `finished_at IS NULL` as active. + +### WR-02: Crawl limits allow runs longer than the job timeout and unbounded memory + +**File:** `models/settings.go:295-300`, `jobs.go:19`, `classes/crawl/crawler.go:200-214`, `classes/crawl/crawler.go:272-282`, `classes/crawl/store.go:19-83`, `classes/crawl/render_chrome.go:318-338` + +**Issue:** + +- **Duration.** `crawl_max_pages` ≤ 50 000 and `crawl_delay` ≤ 5000 ms are validated separately. Many valid combinations exceed the 6 h job timeout: 50 000 × 500 ms is already 6.9 h, and the maximum is 69 h. Any `golem15.seo.manage` holder, including an MCP agent through `seo_update_settings`, can set them, and the crawl then always ends in CR-01. +- **Memory.** Every page's `PageFacts` stays in memory until `finish`: text up to 100 KB, the rendered copy up to another 100 KB, and up to 1000 `Link` structs with parsed `*url.URL`. At 50 000 pages that is several GB. `SavePages` then inserts up to 50 M link rows in one transaction. +- **Sitemap seeding.** All sitemap URLs (up to 500 files × 5 MiB) go into `seen`, `queue` and `Listed` regardless of `MaxPages`, and `checkSitemaps` builds a robots matcher for each listed key. +- **DOM cap.** The 5 MiB cap is applied after `OuterHTML` has already moved the whole DOM over CDP into Go memory. + +**Evidence:** +- settings.go:296-298 for the ranges. +- crawler.go:210-214 pushes every listed key. +- render_chrome.go:336-338 truncates after `OuterHTML` has returned. + +**Fix:** +- Validate that `crawl_max_pages × crawl_delay` stays well under the job timeout, or raise the timeout from it. +- Cap the frontier and `Listed` (for example at `2 × MaxPages`). +- Store pages in chunks as the crawl goes, not all at the end. +- Drop `Text` and `Links` once the per-page checks have consumed them. +- Read the DOM size first (`document.documentElement.outerHTML.length` through `Evaluate`) and skip pages over `MaxDOM`. + +### WR-03: The Chrome request guard allows any port on the site host + +**File:** `classes/crawl/render.go:166-192`, `classes/crawl/fetch.go:122-158` + +**Issue:** `allowRequest` checks the scheme and host but not the port. Page JavaScript, or an ``/`