Derive page path prefixes from the root page, not the instance id

docs/trailhead/tools.md

@@ -432,8 +432,8 @@ confirm nothing went stale.
 
 ## repair_instance_page_slugs
 
-Realigns an instance's **root page slug** with its identifier. Run it after renaming an
-instance — otherwise every subpage of it 404s.
+Realigns an instance's **root page slug** with its identifier, and carries the page
+revisions along with it. Run it after renaming an instance.
 
 ```bash
 python manage.py repair_instance_page_slugs longmont longmont-2021          # plan
@@ -443,30 +443,35 @@ python manage.py repair_instance_page_slugs --all                           # au
 
 Nothing is written without `--apply`.
 
-### Why a rename breaks the subpages
+### Why a rename used to break the subpages
 
-Page routing keys off the *identifier*, not off `InstanceConfig.root_page`.
-`PathsPage.resolve_url_path` strips `^/<instance-id>(-[0-9]+)?/` from the page's
-`url_path`, and `Query.resolve_page` then prepends the translated root page's `url_path`
-to the path it is handed. They agree only while the root slug equals the identifier — and
-nothing keeps them in step, because the slug is set once at page creation
-(`_create_default_pages`) and never revisited.
+Page routing used to key off the *identifier*. `PathsPage.resolve_url_path` stripped
+`^/<instance-id>(-[0-9]+)?/` from the page's `url_path`, and `Query.resolve_page` prepended
+the translated root page's `url_path` to the path it was handed. They agreed only while the
+root slug equalled the identifier — and nothing kept them in step, because the slug was set
+once at page creation (`_create_default_pages`) and never revisited.
 
-The failure is asymmetric, which is what makes it confusing: **the front page keeps
-working** because its path is empty and survives the mismatch, while every child 404s.
-With identifier `longmont` against slug `longmont-dev`, the strip does not match, so
-`resolve_url_path` returns `/longmont-dev/actions` and `resolve_page` looks up
+The failure was asymmetric, which is what made it confusing: **the front page kept
+working** because its path is empty and survives the mismatch, while every child 404ed.
+With identifier `longmont` against slug `longmont-dev`, the strip did not match, so
+`resolve_url_path` returned `/longmont-dev/actions` and `resolve_page` looked up
 `/longmont-dev/longmont-dev/actions/`.
 
-It has bitten twice: `augsburg-bisko` in July 2026 (an admin slug edit) and both Longmont
+It bit twice: `augsburg-bisko` in July 2026 (an admin slug edit) and both Longmont
 instances in September 2026 (a rename).
 
+Both resolvers now derive the prefix from the root page's own `url_path` (`pages/url_paths.py`),
+so a mismatched slug no longer breaks routing. This command is now hygiene rather than a
+fix — with one exception, the revision pass below, which prevents a *different* regression.
+
 ### What it does
 
 - Sets the primary root page's slug to the identifier, and each **translated** root page's
-  to `<identifier>-<n>`. Translations matter: that `(-[0-9]+)?` in the regex is what makes
-  the Spanish site work, and it fails by the same rule. Translations resolve by
-  `translation_key` and locale, never by slug, so renaming them moves no references.
+  to `<identifier>-<n>`. The suffix is no longer a routing requirement, but Wagtail still
+  needs it: `Page._slug_is_available` compares against all siblings regardless of locale,
+  and translated roots are siblings under the Wagtail root, so they cannot share the primary
+  page's slug. Translations resolve by `translation_key` and locale, never by slug, so
+  renaming them moves no references.
 - Moves every page through a **temporary slug first**. Root pages are siblings and Wagtail
   requires the slug to be unique among siblings, so a rename that swaps two identifiers
   cannot be applied in one pass — the retired instance still holds the slug the new one

src/nodes/management/commands/repair_instance_page_slugs.py

@@ -7,31 +7,31 @@
 
 Nothing is written without ``--apply``.
 
-## Why this is needed at all
-
-Page routing keys off the *identifier*, not off ``InstanceConfig.root_page``.
-``PathsPage.resolve_url_path`` (``pages/page_interface.py``) strips
-``^/<instance-id>(-[0-9]+)?/`` from the page's ``url_path``, and ``Query.resolve_page``
-(``pages/schema.py``) then prepends the translated root page's ``url_path`` to the path it
-is given. The two only agree while the root page's slug equals the identifier.
-
-Nothing keeps them in step. The slug is set from the identifier once, when the pages are
-created (``InstanceConfig._create_default_pages``), and never revisited -- so **renaming an
-instance silently breaks every subpage of it**, and so does editing the root page's slug in
-the Wagtail admin. The failure is a 404 on the children while the front page keeps working,
-because the front page's path is empty and survives the mismatch: with identifier
-``longmont`` and slug ``longmont-dev``, the strip does not match, ``resolve_url_path``
-returns ``/longmont-dev/actions``, and ``resolve_page`` looks up
-``/longmont-dev/longmont-dev/actions/``, which exists nowhere.
-
-This has now happened twice -- ``augsburg-bisko`` in July 2026 via an admin slug edit, and
-both Longmont instances in September 2026 via a rename -- which is why it is a command and
-not a third one-off script.
+## What this was for, and what it is for now
+
+Page routing used to key off the *identifier*. ``PathsPage.resolve_url_path`` stripped
+``^/<instance-id>(-[0-9]+)?/`` from a page's ``url_path`` while ``Query.resolve_page``
+prepended the translated root page's ``url_path``, so the two agreed only while the root
+page's slug equalled the identifier -- which nothing maintained. A rename, or a slug edited
+in the Wagtail admin, silently 404ed every subpage while the front page kept working, twice:
+``augsburg-bisko`` in July 2026 and both Longmont instances in September 2026.
+
+Both resolvers now take the prefix from the root page's own ``url_path`` (see
+``pages/url_paths.py``), so **a mismatched slug no longer breaks routing**. What is left is
+hygiene, and it is still worth having: the slug is what the Wagtail admin shows, it is what
+``_create_default_pages`` falls back to when ``root_page`` was never populated, and a
+drifted slug is a reliable sign that an instance was renamed without anything else being
+looked at. Run it with no ``--apply`` as an audit for that.
+
+The revision pass is the part that is not cosmetic -- see below.
 
 ## Translations are part of the fix
 
-A translated root page carries a numeric suffix (``longmont-1``), which is what the
-``(-[0-9]+)?`` in the regex is for, so the Spanish site breaks and heals by the same rule.
+A translated root page carries a numeric suffix (``longmont-1``). Routing no longer needs
+that suffix, but Wagtail still does: ``Page._slug_is_available`` compares against all
+siblings regardless of locale, and translated roots are siblings under the Wagtail root, so
+they cannot share the primary page's slug.
+
 Translations are resolved by ``translation_key`` and locale
 (``InstanceConfig.get_translated_root_page``), never by slug, so renaming them moves no
 references. Every tree the instance serves is repaired the same way: slug, descendant

src/nodes/models.py

@@ -1685,10 +1685,33 @@ def get_outcome_nodes(self) -> list[NodeConfig]:
         pks = [node.database_id for node in root_nodes if node.database_id is not None]
         return list(self.nodes.filter(pk__in=pks))
 
+    def _find_existing_root_page(self, candidates: models.QuerySet[Page]) -> Page | None:
+        """
+        Find this instance's existing root page among the Wagtail root's children.
+
+        Keyed on ``root_page`` first, because that is the authoritative link and the only
+        one that survives a rename. Looking the page up by ``slug=self.identifier`` alone
+        used to be the whole of it, which meant that once the slug and the identifier had
+        drifted apart -- a rename, or a slug edited in the Wagtail admin -- this found
+        nothing and the caller built a *second* root page beside the real one. That orphan
+        is what left ``augsburg-bisko`` with a duplicate in July 2026, and what put a stale
+        page in a position to squat on the slug ``longmont`` needed in September 2026.
+
+        The slug fallback is kept for instances whose ``root_page`` was never populated.
+        """
+        if self.root_page_id is not None:
+            page = candidates.filter(pk=self.root_page_id).first()
+            if page is not None:
+                return page
+        return candidates.filter(slug=self.identifier).first()
+
     def _create_instance_root_page(self) -> Page:
         from pages.models import InstanceRootPage
 
         root_node: Page = cast('Page', Page.get_first_root_node())
+        existing = self._find_existing_root_page(root_node.get_children())
+        if existing is not None:
+            return existing
         with override(self.primary_language):
             locale, _ = Locale.objects.get_or_create(language_code=self.primary_language)
             page = root_node.add_child(
@@ -1734,9 +1757,8 @@ def _create_default_pages(self) -> Page:  # noqa: C901
         root_node: Page = cast('Page', Page.get_first_root_node())
         with override(self.primary_language):
             locale, _ = Locale.objects.get_or_create(language_code=self.primary_language)
-            try:
-                home_page = home_pages.get(slug=self.identifier)
-            except Page.DoesNotExist:
+            home_page = self._find_existing_root_page(home_pages)
+            if home_page is None:
                 home_page = root_node.add_child(
                     instance=OutcomePage(
                         locale=locale,

src/nodes/tests/test_default_root_page.py

@@ -0,0 +1,94 @@
+"""
+Tests for finding an instance's existing root page rather than building a second one.
+
+The lookup used to be ``home_pages.get(slug=self.identifier)``, so once the slug and the
+identifier had drifted apart it found nothing and its caller created a duplicate root page
+beside the real one. That orphan is half of what made both page-routing incidents hard to
+unpick: ``augsburg-bisko`` ended up with a duplicate in July 2026, and a stale page was left
+holding the slug ``longmont`` needed in September 2026.
+"""
+
+from __future__ import annotations
+
+from wagtail.models import Page
+
+import pytest
+
+from nodes.models import InstanceConfig
+from nodes.tests.factories import InstanceConfigFactory
+
+pytestmark = pytest.mark.django_db
+
+
+def _root_page(ic: InstanceConfig, slug: str) -> Page:
+    from pages.models import InstanceRootPage
+
+    wagtail_root = Page.get_first_root_node()
+    assert wagtail_root is not None
+    page = wagtail_root.add_child(instance=InstanceRootPage(title=ic.name, slug=slug, url_path=''))
+    InstanceConfig.objects.filter(pk=ic.pk).update(root_page=page)
+    ic.refresh_from_db()
+    return page
+
+
+def test_the_root_page_is_found_by_fk_when_the_slug_has_drifted() -> None:
+    """A rename leaves the slug behind but never moves the ``root_page`` link."""
+    ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont')
+    root = _root_page(ic, slug='longmont-dev')
+
+    wagtail_root = Page.get_first_root_node()
+    assert wagtail_root is not None
+    found = ic._find_existing_root_page(wagtail_root.get_children())
+
+    assert found is not None
+    assert found.pk == root.pk
+
+
+def test_the_slug_fallback_still_works_without_a_root_page_link() -> None:
+    """Instances predating the FK being populated have only the slug to go on."""
+    ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont')
+    root = _root_page(ic, slug='longmont')
+    InstanceConfig.objects.filter(pk=ic.pk).update(root_page=None)
+    ic.refresh_from_db()
+
+    wagtail_root = Page.get_first_root_node()
+    assert wagtail_root is not None
+    found = ic._find_existing_root_page(wagtail_root.get_children())
+
+    assert found is not None
+    assert found.pk == root.pk
+
+
+def test_a_sibling_instances_root_page_is_not_claimed() -> None:
+    """
+    The lookup must not reach outside the instance it is called on.
+
+    With the slug fallback in play, an instance with no ``root_page`` and no page of its own
+    must come back empty rather than adopting a neighbour's.
+    """
+    ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont')
+    other = InstanceConfigFactory.create(identifier='longmont-2021', name='Longmont 2021')
+    _root_page(other, slug='longmont-2021')
+
+    wagtail_root = Page.get_first_root_node()
+    assert wagtail_root is not None
+    assert ic._find_existing_root_page(wagtail_root.get_children()) is None
+
+
+def test_creating_the_instance_root_page_twice_does_not_duplicate_it() -> None:
+    """
+    ``_create_instance_root_page`` is reached on every sync, not only at creation.
+
+    It used to ``add_child`` unconditionally, so a drifted slug meant a fresh orphan on each
+    call.
+    """
+    ic = InstanceConfigFactory.create(identifier='longmont', name='Longmont')
+    root = _root_page(ic, slug='longmont-dev')
+
+    returned = ic._create_instance_root_page()
+
+    assert returned.pk == root.pk
+    wagtail_root = Page.get_first_root_node()
+    assert wagtail_root is not None
+    # Scoped to this instance's own page: the Wagtail default "Welcome" page is a sibling.
+    assert wagtail_root.get_children().filter(title=ic.name).count() == 1

src/pages/page_interface.py

@@ -1,13 +1,14 @@
 from __future__ import annotations
 
-import re
 from dataclasses import dataclass
 from typing import TYPE_CHECKING, Self
 
 import graphene
 
 from grapple.types.interfaces import PageInterface as BasePageInterface, get_page_interface
 
+from pages.url_paths import to_instance_path
+
 if TYPE_CHECKING:
     from wagtail.models import Page
 
@@ -75,12 +76,20 @@ def resolve_ancestors(root: Page, info: GQLInfo) -> list[PathsPage]:
         return specific.page.get_visible_ancestors(specific.cache)
 
     @staticmethod
-    def resolve_url_path(root, info: GQLInstanceInfo) -> str:
-        url_path = root.url_path
-        # FIXME: This is a dirty way to work around the issue of the slug having the form <instance>-1 or so for translated
-        # pages.
-        # Replace instance ID, optionally followed by a `-` and a number, if it is surrounded by slashes, by a single slash
-        url_path = re.sub('^/%s(-[0-9]+)?/' % re.escape(info.context.instance.id), '/', root.url_path)
-        if len(url_path) > 1:
-            url_path = url_path.rstrip('/')
-        return url_path
+    def resolve_url_path(root: Page, info: GQLInstanceInfo) -> str:
+        """
+        Expose the page's path relative to its instance's root page.
+
+        The prefix comes from the root page's own ``url_path``, which is the same value
+        ``Query.resolve_page`` prepends when the front end hands the result back, so the
+        round trip holds whatever the root page's slug happens to be. See
+        ``pages.url_paths`` for why that matters.
+
+        ``for_page`` locates the instance by treebeard ``path`` prefix rather than by slug,
+        so it cannot drift either.
+        """
+        cache = info.context.cache.for_page(root)
+        root_page = cache.translated_root_page if cache is not None else None
+        if root_page is None:
+            return root.url_path
+        return to_instance_path(root.url_path, root_page.url_path)

src/pages/schema.py

@@ -11,6 +11,7 @@
 from nodes.models import InstanceConfig
 from nodes.schema import NodeType
 from pages.page_interface import PageInterface
+from pages.url_paths import to_url_path
 
 from .models import OutcomePage, PathsPage
 from .perms import PagePermissionPolicy
@@ -85,14 +86,13 @@ def resolve_pages(
     @staticmethod
     def resolve_page(query, info: GQLInstanceInfo, path: str, **kwargs) -> Page | None:
         qs = Query.resolve_pages(query, info, **kwargs)
-        if not path.endswith('/'):
-            path = path + '/'
-        # Prepend the url_path of the translated root page
         instance_config = InstanceConfig.objects.get(identifier=info.context.instance.id)
         root_page = instance_config.get_translated_root_page()
-        path = root_page.url_path.rstrip('/') + path
+        # Inverse of `PathsPage.resolve_url_path`, which handed this path out; both take the
+        # prefix from the root page's `url_path`. See `pages.url_paths`.
+        url_path = to_url_path(path, root_page.url_path)
         for page in qs:
-            if page.url_path == path:
+            if page.url_path == url_path:
                 return page
         return None