epic: package work for the first two deployed-editor go-lives

[jpslav]

Standing integration branch for a go-live epic: getting CanopyCMS to a deployed
editor for the first two real adopter sites — a documentation/knowledge-base site
first, then a marketing site.

Kept in draft. This body is the running ledger — it is rewritten on every merge, so it
always answers "what is in this branch right now" without reading the commit log.

Why this epic

Both adopter sites run Canopy in mode: 'dev' locally and neither wires
CanopyCmsService. The thing that has never happened is a real site running the
deployed editor. This branch closes the package-side half of that gap.

Two independent audits of those sites found they had converged on the same missing
capabilities — separately building search indexing, sitemap/SEO metadata, markdown
rendering, heading-annotation and table-of-contents handling, entry-filename parsing,
and trailing-slash URL normalization for static exports. Both repos had independently
created a file with the same name to solve the heading problem. That convergence is the
ranking signal for what belongs in the package: when two teams with no contact build
the same thing, the package is missing it.

Everything here is generally applicable — none of it is specific to those two sites.

Landed

All package work for this epic is merged. Full suite green on the merged result:
4064 tests passing, typecheck / lint / client-bundle-boundary / backlog-integrity
all clean.

Change	Notes
docs/adopter-migration.md	The adopter-facing record. Every entry names the pattern of code it supersedes, never a specific adopter's files
Backlog re-baseline	Corrected an inverted premise in the task index; filed task files for adopter requests and audit findings that had none
required: false infers an optional property	Breaking, type-level. Was a required T | undefined, now field?: T. Only an explicit required: false is affected — omitting required still infers required, pinned by tests in both directions
parseTypedFilename + defaultBuildPath exported	Both existed internally and were merely unreachable. Four hand-rolled copies of the filename parser were found across two audited adopter repos, disagreeing on segment count and case-folding
read/readByUrlPath return meta.entryType + meta.entryId	Already resolved internally; this is plumbing. Enables routing on entry type instead of re-parsing a path
listEntries carries updatedAt	The stat already happened and was discarded. Documented as filesystem mtime, so a fresh CI clone reports checkout time
Sitemap + SEO metadata helpers	Enumerates every routable entry type by default, so omission requires explicit opt-out. Ships as one change because noindex is a single predicate feeding both the page's robots directive and sitemap exclusion
Block-registry types, field fragments, shared-block recipe	Exhaustive block→component mapped type instead of a renderer component; defineFieldFragment; documented shared-block recipe with its caveat
Build-context factory, resolveEntryTitle, toPlainText	The three primitives genuinely duplicated across adopters

Two things reviewers should look at

The one breaking change is the optional-property inference. The break is narrow and
one-directional: reading, literal construction, keyof, in, and spreads are all
unaffected — only assignment to a hand-written interface declaring the key as
required-with-undefined, Required<T>, and exactOptionalPropertyTypes projects.
Measured in-repo blast radius was 2 lines.

meta.entryType is inferred, not read, for legacy files that predate embedded-ID
filenames: it reports the collection's default entry type, which may not be what the
file is. meta.entryId === undefined is the exact tell. Documented rather than changed,
since adopters are being encouraged to branch routing on that field.

Deliberately not built

• A bundled markdown renderer. Would add three runtime dependencies and, to dodge
  the react-markdown-in-RSC trap, would have to be a client component — forcing every
  adopter's markdown fields into the client bundle. It could not cover the MDX half
  anyway, and the divergence that prompted the request (whether external links open in a
  new tab) is site policy, not CMS policy.
• A generic search-document extractor. Comparing two real implementations, the
  derivation logic shares nothing but "walk strings, skip structural keys". A helper
  guessing which keys are prose would silently drop content from a search index — the
  same silent-divergence failure the request was raised about, relocated somewhere
  adopters cannot see it. The genuinely duplicated plumbing shipped instead.
• A renderBlocks() component. It would have to fix a key strategy, an
  unknown-template policy and prop threading; any of those choices makes it unusable for
  someone. The type plus a documented loop is the better trade.

Decisions taken during this epic

Two were settled deliberately rather than left to the code, and they reinforce
each other:

Publish state is branch-only. No per-entry draft/published field. Merged to the
base branch ⇒ public; unmerged ⇒ not public; there is no third state. noindex means
public-but-unadvertised, not hidden. This is what the architecture always implied — the
gap was that nothing stated the negative half, so an adopter invented a draft
convention that lived in its docs and not in its code, and silently rotted. No
enumeration helper may invent a publish filter; the helper that shipped here is named
collectRoutableEntries for exactly that reason.

Path ACLs are enforced on the request-scoped listing, rather than deferring by
restricting listing to build time. This is what makes the first decision load-bearing:
with publish state branch-only there is no draft flag to fall back on, so on an unmerged
branch every entry is unpublished by definition and a request-time reader would see all
of it. That makes the exposure sharper, not weaker — which is why filtering won over the
cheaper deferral.

defaultBranchAccess: 'deny' is now genuinely usable, and is what canopycms init
scaffolds. Previously the generator wrote 'allow' while the config schema defaulted
to fail-closed 'deny', so every scaffolded project opted out of secure-by-default. The
reason nobody had simply flipped it was that 'deny' did not work: the client granted a
branch's creator unconditionally while the server required an ACL match first, so under
'deny' the creator of a branch saw an enabled Submit and got a 403. Both halves are
fixed together — flipping the default without closing that divergence would have turned
a latent bug into a day-one one.

Review

Three rounds, all independent of the authors: one broad pass, four parallel specialists
(authorization, types, static generation, docs), then a final broad pass judging the
result as a whole. Every finding is fixed and merged. Rounds two and three each found a
defect introduced by the previous round's fixes, which is the case for having run them in
sequence.

The findings worth knowing about, because each was a silent failure rather than a broken
one:

• A content file whose name did not parse was skipped through a debug-gated log, so a
  page could vanish from both the build and the sitemap with no output at all. Now a
  red build — and then narrowed, because the first fix also red-built a plain README and
  the sibling-artifact layout the README itself tells adopters to commit.
• defineSeoFieldGroup({ group }) typed its wrapper key as required while the runtime
  treated it as optional, so data.seo.metaTitle type-checked and crashed on real
  content omitting the key.
• The block-registry recipe crashed on data at rest: compile-time exhaustiveness covers
  the schema, not content carrying a renamed template. Confirmed as an HTTP 500 under
  next dev.
• generateContentSitemap accepted a relative siteUrl, producing a sitemap search
  engines silently reject.
• The noindex predicate had two independently-configured call sites and could disagree
  with itself; the field location is now set once on the context and bound into both.

CodeQL then caught one thing all three rounds missed: a polynomial-ReDoS regex in
siteUrl normalization, introduced with the sitemap helper. Measured at 2377ms versus
0.013ms on a 40k-slash input; replaced with a linear scan and pinned by a regression
test. Worth noting that static analysis found what three careful readings did not.

A normal merge commit is fine. An earlier draft of this body asked for a squash, on
the grounds that the branch's history carried detail that should not reach main. That
is no longer true — the branch history was rewritten and every commit, message and tree,
was verified clean. Squashing would now only cost the per-change commits, which are worth
keeping for git bisect and for reading the epic later.

Review guidance

Every change carries an entry in docs/adopter-migration.md naming what an adopter must
do and what local code the change makes deletable. That column is the deliverable: an
upgrade that adds a new API without removing what it supersedes leaves two
implementations to drift apart, which is the failure mode most of these changes exist to
end.

Each implementation branch was reviewed cold by an independent reviewer that did not
write it and re-ran the suite itself. That review confirmed the branches via exact test
arithmetic and a reverse-red proof, and found two real defects (a filename parser
accepting dotfiles, and the inferred-entryType gap above), both since fixed.

[debshila]

Review — PR #235 (epic: package work for the first two deployed-editor go-lives)

Branch: integration-202608-b → main · Scope: 96 files, +7,782 / −539, 34 commits
Reviewed at: 61c6e95 (merge base db4f871 = origin/main exactly — a clean fast-forward, zero commits on main not in this branch, so #229's landed work is already underneath this)
CI: all 10 checks green (validate/typecheck/test, dual-build, 4× e2e shards, merge-report, CodeQL ×3). One automated review (github-advanced-security); no human review, no comments.

---

Verdict

Approve with one fix first. This is the most adopter-facing epic in the repo so far and the shape of it is right: every change is generally applicable, docs/adopter-migration.md names what each one makes deletable, and the two deliberate decisions (publish-state-is-branch-only, ACLs enforced on the request-scoped listing) are load-bearing on each other rather than independently chosen. The authorization work in particular is the strongest part — checkBranchAccessWithDefault's new precedence ladder is correct at every rung I traced, and the tests pin the three cases that matter (creator wins over bare default, explicit allowlist wins over creator, managerOrAdminAllowed wins over both).

One finding should land before merge:

• #1 — a confirmed, measured polynomial ReDoS in toPlainText's TAG_RE, added in this PR. 26.3 seconds on a 128 KB body, quadratic scaling, and the trigger is an unterminated <Tag followed by a run of whitespace — a plausible shape in real MDX. I verified a one-token fix that removes the blowup entirely (26,321 ms → 0.63 ms) with identical output on nine tag shapes.

The PR body closes with: "CodeQL then caught one thing all three rounds missed: a polynomial-ReDoS regex in siteUrl normalization… Worth noting that static analysis found what three careful readings did not." That lesson generalizes further than the PR takes it. Of my top three findings, all three are the same defect class as that CodeQL alert, all in this PR, and none was flagged: the regex above (#1), the startsWith('//') off-site check that #229 removed from sanitizeHref two weeks ago and this PR reintroduces in isAbsoluteUrl (#2), and the exact /\/+$/ pattern CodeQL flagged — still present, unchanged, in the example app that adopters copy (#3). One fix was applied where the alert pointed and nowhere else.

Everything else is follow-ups.

---

Coverage — what this review actually covered

Read in full and reasoned about:

• authorization/branch.ts + authorization/types.ts (the whole precedence ladder), protected-branch.ts for the grant it keys off
• content-listing.ts (parseTypedFilename, looksLikeMalformedEntry, the build-mode throw, updatedAt), content-tree.ts (visibility threading + collection pruning)
• context.ts (resolveVisibilityImpl and its three short-circuits), services.ts, build-canopy.ts
• static/seo.ts (whole file, adversarially), static/index.ts, canopycms-next/src/static.ts, context-wrapper.ts
• entry-schema.ts (the optional-property inference, defineSeoFieldGroup overloads, BlockComponentRegistry)
• content-reader.ts (ContentReadMeta), ai/to-plain-text.ts (regex pipeline)
• docs/adopter-migration.md (build-guard + SEO sections in full, the rest skimmed for claims), the README's new sections, all four canopycms.config.ts files and the CLI template, apps/example1/app/{lib/canopy.ts,sitemap.ts}

Verified by running code rather than reading (details in the findings): the TAG_RE timings and the candidate fix; isAbsoluteUrl/resolveSeoUrl/withTrailingSlash against eight URL spellings including WHATWG backslash forms, checking what a browser actually resolves each output to. I did not run the full suite or the e2e shards — my local node_modules is missing several packages, and CI's green is the evidence for those.

Not covered: the 14 new/moved .claude/future-tasks/ files and program-log.md were read for claims about the code, not audited as documents; apps/example1/app/components/PostView.tsx; the large new test files (static.test.ts, entry-schema.test.ts, content-listing.test.ts, context.test.ts, to-plain-text.test.ts) were read for what they assert, not line-read; CODEBASE_GUIDE.md. Treat absence of findings there as "not looked at".

---

Claims I verified independently

Claim	Result
'deny' is now genuinely usable — creator and base-branch grants close the divergence	Holds, and the precedence is right at every rung. Creator is scoped to the no-ACL case (an explicit allowlist omitting them still denies — pinned by a test), managerOrAdminAllowed short-circuits ahead of both, and the base-branch grant sits below defaultAccess so it can never replace allowed_by_acl or override denied_by_acl. The four new branch.test.ts cases cover exactly these.
The base-branch grant "cannot widen anything dangerous"	Holds on all three named guards. canPerformWorkflowAction disables its system-branch grant on the same flag (so access says yes and the action still resolves false — there is a test for it); getBranchWriteProtection().readOnly blocks prod writes; the path layer still decides reads and defaults to 'deny'.
Path ACLs are enforced on listEntries and buildContentTree, including meta.indexEntry	Holds. The predicate is applied in listEntries before extract runs, and buildContentTree routes every read through listVisibleEntries, so a denied index entry never reaches extract. The "collections whose children are all filtered out are pruned" claim is real code (if (allChildren.length === 0) return null), not just a doc sentence.
The ContentVisibilityOptions predicate is not adopter-supplyable	Holds. It is a separate positional parameter on listEntries/buildContentTree, not a key on ListEntriesOptions/BuildContentTreeOptions, and context.ts is the only producer. An adopter cannot widen or override it through the public options object.
The three visibility short-circuits are load-bearing, not just an optimization	Holds. createContentAccessChecker calls getSettingsBranchRoot(), which provisions/clones the settings workspace — so for createBuildCanopy (a standalone script, where neither isBuildMode() nor isDeployedStatic fires) the user === STATIC_DEPLOY_USER guard is the only thing preventing a settings-workspace clone the script never needed.
required: false → optional property; omitting required stays required	Holds. The two-mapped-type intersection keys on F['required'] extends false, which boolean | undefined does not satisfy, so the three-way distinction is real. Simplify is the standard homomorphic idiom and preserves ?. Both directions are pinned in entry-schema.test.ts.
defineSeoFieldGroup({ group })'s wrapper key is now optional, matching the runtime validator	Holds — NestedSeoFieldGroup carries required: false, and the flat variant is type: 'group', which FlattenInlineGroups flattens so the seven fields land at top level as optional. Schema names come from DEFAULT_SEO_FIELD_NAMES, the same constant extractSeoFields reads, so the two ends cannot drift.
noindex is derived from ONE predicate feeding both surfaces	Holds structurally, not just by convention: entryToMetadata and generateContentSitemap both call isNoindexEntry, and mergeSeoFieldLocation binds the field location from one context-wide default with a deliberate key-by-key merge (a naïve spread would clobber it with undefined, and the comment says so).
generateContentSitemap is bound to the build context, so ACL filtering never empties a sitemap	Holds. boundGenerateContentSitemap calls getCanopyForBuild(), whose user is STATIC_DEPLOY_USER → visibility short-circuits to {}. A request-time reach on a prod server throws via guardBuildContext instead of silently returning a filtered sitemap.
resolveEntryTitle is client-safe enough to re-export from the root entry	Holds — utils/title-field.ts's only imports are FieldConfig/InlineGroupFieldConfig types, erased at compile time.
basePath no longer silently drops entries under shape: 'single'	Holds — the filter is now gated on shape !== 'single', matching the option's own documented "no effect with shape: 'single'".
The ReDoS fix in stripTrailingSlashes is correct	Holds — a linear character scan, no regex. See finding #3 for where it did not get applied.

---

Findings

1. HIGH — toPlainText's TAG_RE is a polynomial ReDoS (measured: 26.3s on 128 KB)

packages/canopycms/src/ai/to-plain-text.ts:69

const ATTR_CONTENT = `(?:[^<>"']|"[^"]*"|'[^']*')*`
const TAG_RE = new RegExp(`<\\/?([A-Za-z][\\w.-]*)?(?:\\s${ATTR_CONTENT})?\\s*\\/?>`, 'g')

ATTR_CONTENT's [^<>"'] already matches whitespace, and the \s* that follows it matches whitespace too. Every whitespace character after an unterminated tag can therefore be split between the two in any proportion, so when the closing > never arrives the engine enumerates every split. Measured on '<Callout\n' + '\n'.repeat(n) + 'text':

input size	current	with \s* removed
8 KB	102 ms	0.28 ms
32 KB	1,691 ms	0.11 ms
128 KB	26,322 ms	0.63 ms

Clean quadratic (~16× per 4× input). Prose after the stray tag is not a trigger (330 KB of ordinary paragraphs: 2.6 ms) — the tail has to be whitespace-dominated, i.e. an unterminated <Tag followed by blank lines or a long indented block. That is an ordinary content-authoring accident in MDX (a literal < before a word, a typo'd component open), not a crafted payload.

Why this is worth blocking on rather than filing: it is the same defect class the PR's own review section says CodeQL caught and three review rounds missed, introduced in the same PR, on an input the PR's own stripTrailingSlashes comment already argues counts as uncontrolled ("siteUrl is adopter-supplied and can reach here from config or an env var"). Entry bodies are at least as uncontrolled as an env var, and toPlainText is pitched at search-index builders — i.e. code that walks every entry, so one bad file stalls the whole index build or next build. CodeQL is green here, so the alert that motivated the siteUrl fix does not cover this.

Fix, verified: delete the redundant \s*.

const TAG_RE = new RegExp(`<\\/?([A-Za-z][\\w.-]*)?(?:\\s${ATTR_CONTENT})?\\/?>`, 'g')

ATTR_CONTENT already absorbs the trailing whitespace, so nothing is lost. I checked output parity on <br />, <a>, </a>, <a >, <Foo bar="1 > 2" />, <>, </>, <Callout>hi</Callout>, <a\nhref="x"> — identical in every case. A regression test in the shape of the siteUrl one (assert a bounded runtime on a 128 KB whitespace tail) pins it.

2. MEDIUM — isAbsoluteUrl reintroduces the startsWith('//') off-site check that sanitizeHref removed two weeks ago

packages/canopycms/src/static/seo.ts:158

export function isAbsoluteUrl(url: string): boolean {
  return /^[a-z][a-z0-9+.-]*:\/\//i.test(url) || url.startsWith('//')
}

utils/sanitize-href.ts, already on main under this branch, carries this comment:

This is enforced by checking whether the input DECLARES a scheme, not by matching a // prefix: WHATWG URL treats backslash as equivalent to slash for special schemes, so /\evil.com, \\evil.com and \/evil.com are all protocol-relative in effect. An earlier version of this function checked startsWith('//') and let all three through…

The same spellings defeat isAbsoluteUrl. Measured, with each output resolved the way a browser would resolve it against the page origin:

entry field value	isAbsoluteUrl	resolveSeoUrl(v) (no siteUrl)	browser resolves to
/\evil.com	false	/\evil.com	https://evil.com/
\\evil.com	false	/\\evil.com	https://evil.com/
\/evil.com	false	/\/evil.com	https://evil.com/
/\t/evil.com	false	/\t/evil.com	https://evil.com/

entryToMetadata documents omitting siteUrl as a supported mode ("Omit to leave them relative and let Next's metadataBase resolve them"), and that is exactly the path where this bites: an entry's canonical or ogImage reaches <link rel="canonical"> / og:image / openGraph.url pointing at an attacker-controlled origin. With a siteUrl supplied the value is prefixed and stays same-origin, so the exposure is the no-siteUrl configuration specifically.

The input is CMS-author-supplied, so this is not an anonymous XSS — it is SEO poisoning (a canonical tag handing your ranking to another origin) and a social-card/referrer leak, reachable by anyone with write access to one entry. Lower severity than #229's open redirect; identical mechanism.

There is a second, unrelated consequence of the same predicate: //cdn.example.com/x in extraUrls returns true, passes through verbatim, and lands in the sitemap as a non-absolute <loc> — the precise invalidity generateContentSitemap throws about for siteUrl ("search engines silently reject the entire file"), just not checked for the per-URL values.

Fix direction: apply sanitizeHref's own conclusion — test whether the input declares a scheme (/^[a-z][a-z0-9+.-]*:/i) and treat anything else that the URL parser resolves off-origin as not-absolute-but-not-safe-either. Better still, have one of the two functions call the other; two independently-maintained answers to "is this URL off-site" in one package is the drift this whole epic is about.

3. MEDIUM — the ReDoS fix was applied at the alert site only; the example app still ships the flagged pattern

apps/example1/app/lib/canopy.ts:90

export const SITE_URL = (process.env.NEXT_PUBLIC_SITE_URL ?? 'http://localhost:3000').replace(
  /\/+$/,
  '',
)

That is character-for-character the pattern the PR body describes replacing:

a polynomial-ReDoS regex in siteUrl normalization, introduced with the sitemap helper. Measured at 2377ms versus 0.013ms on a 40k-slash input; replaced with a linear scan and pinned by a regression test.

Same operation, same input (siteUrl), same env-var provenance the stripTrailingSlashes comment cites as the reason it counts as uncontrolled — in the file this repo holds up as the reference integration. CodeQL did not flag it (checks are green), which is the useful signal here: the alert was treated as the bug rather than as one instance of it.

The mechanical cause is that the fix was made unshareable: stripTrailingSlashes is a module-private function in seo.ts, not exported and not re-exported from canopycms/server, so there is no package-provided way to do this correctly. apps/example1 had to re-roll it, and so will every adopter who follows the README's SITE_URL pattern. By the PR's own ranking signal — "when two teams with no contact build the same thing, the package is missing it" — this qualifies before anyone has built it twice.

Fix direction: export stripTrailingSlashes (or fold it into resolveSeoUrl's contract so callers never need it) and use it in apps/example1. Practically the exposure here is nil — build-time, builder-controlled env var — but the example is copy-paste material, and shipping the flagged pattern in it while the changelog says it was replaced is the kind of claim/code gap this repo audits for.

4. MEDIUM — the unparseable-filename build guard misses the shape most likely to be a lost page

packages/canopycms/src/content-listing.ts:336, 468

The guard's stated purpose is that a page must never disappear from a build with no output. looksLikeMalformedEntry gates it on filename.split('.').length >= 4, reasoning that a valid entry always has four segments so anything shorter "could never have parsed as an entry regardless of its content".

That is true about parsing and false about intent. The likeliest way a file loses its parse is losing its ID segment — a hand-created file, a bad rename, a merge — and post.hello-world.md has three segments. It fails parseTypedFilename (parts.length < 3 after the extension strip), is skipped, and looksLikeMalformedEntry returns false, so it never reaches skippedFiles and the build stays green with the page gone. The guard fires on post.hello-world.BADID.md (wrong ID, 4 segments) but not on post.hello-world.md (no ID, 3 segments) — and the second is the more common accident.

I understand why the narrowing happened: the first version red-built a plain README.md and the documented {contentId}.suffix.ext sibling artifact, and docs/adopter-migration.md records that correction honestly. But segment count is a proxy for "was this meant to be an entry", and it is a proxy that fails on the motivating case. A shape test that also catches {knownEntryTypeName}.{anything}.{ext} would cover the ID-loss case without touching README.md (2 segments, first segment not an entry type) or 5NVkkrB1MJUv.profile.json (first segment not an entry type either).

Worth stating in the same breath: the guard is listEntries-only. buildContentTree calls listCollectionEntries with no onSkip, so an entry lost from the tree is still silent in every mode. .claude/future-tasks/build-content-tree-silent-skip.md files it, which is the right way to ship a partial fix — noting it here so the guard isn't read as covering more than it does.

5. MEDIUM — a freshly scaffolded project renders nothing until the adopter edits the config

packages/canopycms/src/cli/template-files/canopycms.config.ts.template

canopycms init now scaffolds defaultBranchAccess: 'deny' and defaultPathAccess: 'deny'. Flipping the branch default to match the schema is unambiguously right — the generator writing 'allow' against a fail-closed schema was the bug. But the path default now makes the first-run experience a wall: apps/example1 needed exactly this to keep working, and its own comment says why:

Without this, a non-admin visitor (including the dev-auth default user) gets "Forbidden: path access denied" on every route, since defaultPathAccess otherwise falls back to fully closed.

So canopycms init → npm run dev → every page 403s, for the scaffolded developer themselves, until they find defaultPathAccess in the README. That is a strictly worse first run than the pre-flip 'allow'/'allow' pairing, and "secure by default" doesn't require it: the two decisions are separable, and only the branch one was the divergence being fixed.

The template comment does point at README's Public read on server deployments, which is the correct pointer — the gap is that the scaffold's own smoke test is the failure. Three options, in my order of preference: scaffold defaultPathAccess: { read: 'allow' } (public read, edit/review still closed — the posture the template comment itself recommends and apps/example1 adopts); or keep 'deny' and add the working { read: 'allow' } line commented out directly above it; or leave it and make the 403 name the config key, so the first-run failure is self-diagnosing rather than a generic "Forbidden".

Also worth a line in docs/adopter-migration.md: an existing adopter who relied on the old 'allow' default and never wrote defaultPathAccess is unaffected (their config still says what it said), but one who copied the scaffold and later removes the key inherits the closed default. I didn't find this transition covered there.

6. LOW–MEDIUM — the epic's own "don't let two implementations drift" test, applied to the epic

The PR's review guidance is explicit:

an upgrade that adds a new API without removing what it supersedes leaves two implementations to drift apart, which is the failure mode most of these changes exist to end.

Three places in this diff now hold a second implementation of something the package already answers:

1. isAbsoluteUrl vs. sanitizeHref's scheme test (finding #2) — two answers to "is this URL off-site", one already corrected.
2. apps/example1's /\/+$/ vs. stripTrailingSlashes (finding #3) — two answers to "strip trailing slashes", one already corrected.
3. canopycms-cdk's isValidDeploymentName vs. the runtime copy — pre-existing, and handled correctly (a fixture-driven drift test), which is the model the other two are missing.

Nothing here is individually urgent; together they suggest the migration guide's "now deletable" column deserves an internal counterpart — when a fix lands, grep for the pattern it replaced rather than only the file the alert named.

---

Smaller notes

7. Sitemap ordering is locale-dependent. canopycms-next/src/static.ts:216 sorts with a.url.localeCompare(b.url) and no locale argument, while the doc directly above promises "Output is stably ordered … so a rebuild doesn't reshuffle the file." Two build agents with different ICU defaults can emit different orders, producing a spurious diff. localeCompare(b.url, 'en'), or a plain </> comparison, makes the promise true. (This is the same machine-dependence class as the timezone-hardcoded test #229 documented.)

8. mergeSeoFieldLocation has no way to say "flat, just this once". By design an omitted or explicitly undefined group falls through to the context-wide default — correct for the common case, and documented. But there is then no per-call override back to the flat convention; group: '' happens to work (it is falsy, so extractSeoFields reads the entry directly), which is an undocumented accident rather than an API. If per-call opt-out matters, group: null with an explicit check would say it on purpose.

9. entry-schema.ts gained two new dependencies on the isomorphic entrypoint. import type { ComponentType } from 'react' (erased, but it means @types/react must resolve to typecheck canopycms's root entry — relevant given the PR already documents a hard skipLibCheck: true requirement) and a value import of DEFAULT_SEO_FIELD_NAMES from ./static/seo, which pulls the whole ~230-line SEO module into the client bundle graph for one constant. lint:bundle passes and seo.ts is pure, so neither is a break — but the constant would sit more naturally in its own module.

10. createBuildCanopy is a new ACL-bypassing public export. canopycms/server's createBuildCanopy reads as STATIC_DEPLOY_USER and bypasses every branch and path check, with nothing preventing an adopter from calling it inside a server component. The JSDoc is as strong as prose can be (the security note names the misuse and points at getCanopy()), and getCanopyForBuild already had the same property — noting it because this is the first time that capability is a one-call factory an adopter is invited to use, so the footgun is now easier to reach than the guard. A runtime isBuildMode() warning on first use would cost nothing.

11. parseTypedFilename's directory-component precondition is documented but unenforced. The JSDoc is unusually clear that 'foo/bar.slug.<id>.md' parses to type: 'foo/bar' and that callers must path.basename first. Now that it is a public export reachable by adopter code walking a filesystem — which is the stated motivation — a one-line if (filename.includes('/') || filename.includes('\\')) return null would make the contract self-enforcing at no cost. The returned slug carrying the Slug brand without running parseSlug is in the same category: documented, and a trap for the next caller who trusts the brand.

12. Collection metadata is not path-ACL filtered, only entries. buildContentTree's extract(collectionData, …) runs for any collection with at least one visible descendant, and a maxDepth-capped collection returns its node without loading entries at all, so it is never pruned. The context doc says "entries the current user cannot read are omitted", which is accurate as written — flagging it so nobody reads the tree as fully ACL-scoped. Collection paths and .collection.json fields are disclosed structurally.

13. dedupeSitemapItems warns to bare console.warn. Legal — the project-wide no-console allows warn, and canopycms-next is outside the worker-reachable override list. Consistent with the package; noting only because the repo's logging discipline is otherwise strict enough that a reader might expect canopyLogWarn.

---

What's good, specifically

• The precedence ladder in checkBranchAccessWithDefault is documented as a ladder — numbered, highest-first, in the function's own doc — and every rung's scoping rationale is written at the site that could plausibly be "simplified" into a bug ("a short-circuit higher up would replace allowed_by_acl with base_branch and silently strip Withdraw rights"). That is the comment that stops the next refactor.
• The two grants pulling in opposite directions on one flag (isProtectedBranch grants access, disables workflow actions) is genuinely subtle, and both sides say so, each pointing at the other.
• ContentVisibilityOptions as a separate parameter rather than an options key is the right shape, for the reason stated: adopter code must not be able to supply, widen, or override the access check. Easy to get wrong by putting it on ListEntriesOptions where it would have read more naturally.
• docs/adopter-migration.md's "Now deletable" column is the best idea in the epic. Naming the pattern of superseded code rather than a specific adopter's files is what makes it reusable, and the guard-narrowing entry's "you can move it back — relocating it may have silently broken the entryTransforms call that reads it" is the rare migration note that fixes damage the previous version caused.
• Shipping the sitemap and SEO helpers as one change with the reason stated (noindex is one predicate feeding two surfaces) is the correct call, and mergeSeoFieldLocation makes the agreement structural rather than a matter of adopter discipline — including the explicit note on why a naïve spread would silently clobber the default.
• Every-routable-type-by-default, with the mirror failure documented. Both directions are named: the type nobody remembered to add, and the type that has no route. The apps/example1 sitemap then demonstrates all three exclusions with a reason each, including "modelling home as a root index entry instead would remove the need for both lines" — showing the better design next to the workaround.
• updatedAt's caveat is repeated at all four places it surfaces (ListEntriesItem, RoutableEntry, the sitemap option, the README) rather than stated once and assumed. A checkout-time mtime silently becoming a public <lastmod> is exactly the kind of thing one un-caveated call site produces.
• The required: false inference is pinned in both directions, and the README table spells out the three-way distinction plus the runtime/type divergence it does not fix ("typed as present but validated as absent-tolerant") — stating the residual gap rather than implying the change closed it.
• BlockComponentRegistry ships as a type with a documented loop instead of a renderBlocks(), and the README's loop carries the one assertion it needs with the reason it is safe, plus the if (!Component) return null guard and why compile-time exhaustiveness doesn't cover data at rest. That paragraph is the finding from review round three, written where a reader hits it.

---

Merge notes

• Clean fast-forward: git rev-list --count origin/main ^HEAD is 0, so there is nothing to reconcile and no conflict surface. mergeStateStatus: BLOCKED is the review requirement.
• #1 should land on this branch — a one-token regex change plus a bounded-runtime regression test, matching the one already written for stripTrailingSlashes.
• #2 and #3 are small and thematically inseparable from #1; I'd fold them in while the context is fresh, since all three are the same class and #3 is a two-line change once stripTrailingSlashes is exported.
• #4 and #5 are judgment calls that deserve an explicit decision rather than a silent inheritance — #5 in particular changes what every new adopter sees on their first npm run dev.
• The PR body asks for a normal merge commit rather than a squash, with the reasoning (history verified clean, per-change commits worth keeping for bisect). I agree — the commit granularity here is genuinely useful, and this epic is the kind that gets read back.
• One process observation, offered as such: the three review rounds were organized by dimension (authorization, types, static generation, docs), and the three findings above are all the same defect class crossing three of those dimensions — a regex in ai/, a URL predicate in static/, a copy in apps/. A dimension-partitioned review is structurally unlikely to notice a pattern that spans partitions, which may be the more durable lesson than "static analysis found what three careful readings did not" — CodeQL didn't find these either.

[jpslav]

Thank you — this found more than three rounds of my own review did, and the process
observation is the most useful part.

All five actioned, on c8a74576. Full suite green (4126 tests), lint / lint:bundle /
lint:tasks / typecheck clean.

#1 — fixed. Reproduced your measurement independently: 28,443ms → 0.74ms on 128KB.
Redundant \s* removed, parity re-checked on your nine tag shapes, bounded-runtime
regression test added in the same shape as the stripTrailingSlashes one.

And a second, worse one your review didn't reach, found by sweeping the class rather
than the site: the fenced-code regex /^(```|~~~).*\n([\s\S]*?)\n\1\s*$/gm, present in
both to-plain-text.ts and transform-components.ts — 9,041ms on ~530KB of
unclosed fence openers. Replaced with a linear line scan using a precomputed
nearest-closer lookup, output verified byte-identical across 13,000+ fuzz trials.

#2 — fixed, by the route you preferred. isAbsoluteUrl and sanitizeHref now share
one answer: declaresScheme, isImplicitlyOffOrigin, neutralizeImplicitOffOrigin are
exported from utils/sanitize-href.ts and consumed by static/seo.ts. Confirmed via
Node's URL parser that the backslash spellings now stay same-origin with and without
siteUrl, while a literal //cdn… still passes through verbatim. Your note that
extraUrls can still land a protocol-relative <loc> is filed separately rather than
folded in silently.

#3 — fixed, and you were right about the shape of my mistake. I fixed the line CodeQL
named and swept only packages/**, which structurally could not see apps/. Repo-wide
grep now: no other live /\/+$/. Two structurally similar patterns in cli/migrate.ts
and assets/keys.ts are safe today only because upstream hyphen-collapsing bounds their
input — benchmarked to confirm, and filed, since that safety depends on step ordering
rather than on anything guaranteeing it.

#4 — fixed. Segment count replaced by an entry-type test, so a file that lost its ID
is caught. Four real next builds: post.hello-world.md FAILS (named in the error),
post.hello-world.BADID.md FAILS, README.md + a {contentId}.profile.json sibling
together BUILD CLEAN (17/17 pages).

#5 — applied your first-preference option. Template now scaffolds
defaultPathAccess: { read: 'allow' }, with a pinning test and the migration-guide note
about an adopter who later removes the key.

---

On the process observation — you're right, and it corrects a lesson I'd drawn too
narrowly. I'd concluded "static analysis found what three careful readings did not." The
sharper version is yours: my rounds were partitioned by dimension, your top three
findings are one defect class appearing once in each of three dimensions, and a
dimension-partitioned review is structurally unable to notice that — each reviewer sees
one instance and judges it local. CodeQL didn't find them either; it caught a fourth
instance in a fifth location.

The second ReDoS above is that lesson applied: it was found by asking "where else does
this class live", not by reading ai/ again.

Two incidental findings from the build verification, both filed:
next build reads .canopy-dev/content-branches/<branch>/ even at build time under
mode: 'dev', which contradicts DEVELOPING.md — three agents lost time to it before
this one did; and the extraUrls protocol-relative <loc> case from #2.

Follow-ups #6–#13 are filed to .claude/future-tasks/ rather than actioned here.