fix: remove stale lookupId parsing fallback and tighten malformed lookupId validation

Co-authored-by: nicolehaugen <10600161+nicolehaugen@users.noreply.github.com>

Author: Copilot

docs/reference/presets.md

@@ -220,7 +220,7 @@ Every command, template, and script contributed by a preset (or an extension, or
 
 Identifiers are computed on demand from author-declared manifest content and are never persisted to `.specify/` or any cache. Copying a preset to another machine (or touching its files) does not change the identifiers it produces.
 
-`PresetResolver.collect_all_layers()` returns layer dicts that each include a `lookupId` field that identifies the layer. For manifest-declared preset and extension layers, the `lookupId`'s `sourceId` component is the resolver's registry key or directory name, which can differ from the `id` used by `PresetManifest.iter_contributions()` / `ExtensionManifest.iter_contributions()` (the manifest-declared `id:` field). Both values follow the same grammar, but they only join directly when the installed directory name matches the manifest-declared id — so consumers should treat `lookupId` as the resolver's stack identity and use `layer_kind_from_lookup_id` / the manifest APIs when they need to reason about the originating contribution. Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they carry a synthetic `project:_:{kind}:{name}` `lookupId` that intentionally does not match any manifest contribution.
+`PresetResolver.collect_all_layers()` returns layer dicts that each include a `lookupId` field that identifies the layer. For manifest-declared preset and extension layers, the `lookupId`'s `sourceId` component is the manifest's validated `id:` field, so it joins directly to the `id` used by `PresetManifest.iter_contributions()` / `ExtensionManifest.iter_contributions()` even when the installed directory was renamed. Convention-only layers (undeclared in any manifest) have no manifest id to consult, so their `lookupId`'s `sourceId` falls back to the resolver's registry key or on-disk directory name instead — consumers should treat `lookupId` as the resolver's stack identity and use `layer_kind_from_lookup_id` / the manifest APIs when they need to reason about the originating contribution. Project-local overrides in `.specify/templates/overrides/` are a resolver-only concept — they carry a synthetic `project:_:{kind}:{name}` `lookupId` that intentionally does not match any manifest contribution.
 
 For the full grammar, including the hook name-component convention and last-write-wins deduplication used by extensions, see the [Extension API Reference — Contribution Identifiers](../../extensions/EXTENSION-API-REFERENCE.md#contribution-identifiers) section.
 

extensions/EXTENSION-API-REFERENCE.md

@@ -863,7 +863,7 @@ satisfied = version_satisfies("1.2.3", ">=1.0.0,<2.0.0")  # bool
 
 ## Contribution Identifiers
 
-Every command, template, script, and hook contributed by an extension (or a preset, or the core layer) is addressable at read time by a deterministic opaque identifier. Resolved artifact-stack layers carry a matching `lookupId` field that shares this grammar and identifies the layer's stack position. Manifest contribution `id` values and resolver `lookupId` values are **not always equal** — they only join directly when the installed directory name matches the manifest-declared `id:` (see [Determinism guarantees](#determinism-guarantees) below). Identifiers are **computed on demand from author-declared manifest content** and are **never persisted** to `.specify/` or to any cache file.
+Every command, template, script, and hook contributed by an extension (or a preset, or the core layer) is addressable at read time by a deterministic opaque identifier. Resolved artifact-stack layers carry a matching `lookupId` field that shares this grammar and identifies the layer's stack position. Manifest-declared preset and extension layers use the manifest's validated `id:` for `lookupId`'s `sourceId` component, so their `lookupId` joins directly to the matching `iter_contributions()` entry even after the installed directory is renamed; convention-only contributions have no manifest `id:` to consult and fall back to the on-disk directory / registry key instead (see [Determinism guarantees](#determinism-guarantees) below). Identifiers are **computed on demand from author-declared manifest content** and are **never persisted** to `.specify/` or to any cache file.
 
 ### Grammar
 
@@ -900,11 +900,11 @@ Project-local overrides in `.specify/templates/overrides/` are a resolver-only c
 
 `ExtensionManifest.iter_contributions()` yields dicts of the form `{layer, sourceId, kind, name, id, ...author-declared fields}`; each entry's `id` is the computed identifier. `ExtensionManifest.contribution_id(kind, name)` returns the id for a single lookup, or `None` if no contribution matches. `PresetManifest` exposes the same two methods.
 
-`PresetResolver.collect_all_layers()` returns layer dicts that include a `lookupId` field for every layer type (`project override`, preset, extension, core, and bundled core). Resolver `lookupId` values identify the layer by the resolver's registry key or directory name, which can differ from the manifest-declared source id used by `iter_contributions()`.
+`PresetResolver.collect_all_layers()` returns layer dicts that include a `lookupId` field for every layer type (`project override`, preset, extension, core, and bundled core). Manifest-declared preset and extension layers use the manifest's validated `id:` as the `lookupId` source id, so it matches the id `iter_contributions()` yields for that same contribution. Convention-only layers (no manifest entry declares the contribution) have no manifest id to consult, so their `lookupId` falls back to the resolver's registry key or on-disk directory name.
 
 ### Determinism guarantees
 
-Manifest contribution identifier derivation reads only the in-memory declared manifest content. No filesystem paths, no `os.environ`, no timestamps, and no file-content hashes contribute to those manifest ids. Copying an extension or preset to a different machine (or touching its files) does not change the identifiers it produces. Resolver `lookupId` values are stack identifiers, not manifest contribution ids: for example, an unregistered extension's directory name is the resolver source id, so renaming that directory changes its `lookupId`.
+Manifest contribution identifier derivation reads only the in-memory declared manifest content. No filesystem paths, no `os.environ`, no timestamps, and no file-content hashes contribute to those manifest ids. Copying an extension or preset to a different machine (or touching its files) does not change the identifiers it produces. Manifest-declared resolver `lookupId` values share this stability — renaming the installed directory of a preset or extension that declares an `id:` does not change its `lookupId`. Only convention-only contributions (undeclared in any manifest) derive their `lookupId` from the on-disk directory name or registry key, so renaming that directory does change their `lookupId`.
 
 ### Opacity guidance
 

src/specify_cli/_identifier.py

@@ -108,9 +108,23 @@ def layer_kind_from_lookup_id(lookup_id: str) -> str | None:
     This is the single place that knows the set of valid layer prefixes, so
     consumers can classify a lookupId without re-deriving the grammar via
     string-prefix checks of their own.
+
+    Validates the complete shape, not just the presence of a layer prefix:
+    named contributions require exactly the four ``{layer}:{sourceId}:{kind}:
+    {name}`` components, and hook contributions require exactly the five
+    ``{layer}:{sourceId}:hook:{eventName}:{command}`` components, with every
+    component non-empty. A value such as ``"core:not-an-id"`` or ``"preset:x"``
+    has a recognized layer prefix but the wrong number of components, so it is
+    malformed and returns ``None`` rather than being treated as authoritative.
     """
-    layer, _, rest = lookup_id.partition(":")
-    if not rest or layer not in _LAYER_KINDS:
+    parts = lookup_id.split(":")
+    if len(parts) < 4 or any(not part for part in parts):
+        return None
+    layer = parts[0]
+    if layer not in _LAYER_KINDS:
+        return None
+    expected_len = 5 if parts[2] == "hook" else 4
+    if len(parts) != expected_len:
         return None
     return layer
 

src/specify_cli/artifacts/__init__.py

@@ -407,13 +407,14 @@ def _derive_manifest_path(layer: dict[str, Any], project_root: Path) -> str | No
     Only ``preset`` and ``extension`` layers have an on-disk manifest — core
     and project-override layers return ``None``.
 
-    Since the resolver may set the ``lookupId``'s ``sourceId`` component to
-    the manifest-declared ``id:`` (which can differ from the on-disk directory
-    name for renamed packs), the on-disk directory is read from the layer's
-    explicit ``preset_id`` / ``pack_dir`` (preset layers) or
-    ``extension_id`` / ``extension_dir`` (extension layers) keys, falling
-    back to the ``lookupId`` ``sourceId`` only when those explicit keys are
-    absent.
+    The resolver may set the ``lookupId``'s ``sourceId`` component to the
+    manifest-declared ``id:`` (which can differ from the on-disk directory
+    name for renamed packs), so ``lookupId`` is never parsed for the on-disk
+    directory here. The on-disk directory identity is read exclusively from
+    the layer's explicit provenance keys — ``preset_id`` / ``pack_dir`` for
+    preset layers, ``extension_id`` / ``extension_dir`` for extension
+    layers — which ``collect_all_layers()`` always sets alongside
+    ``lookupId``. Missing provenance keys mean no manifest path is available.
 
     Uses ``as_posix()`` so the string is stable across Windows and POSIX — a
     caller comparing snapshots between operating systems gets the same value
@@ -423,36 +424,26 @@ def _derive_manifest_path(layer: dict[str, Any], project_root: Path) -> str | No
     layer_kind = layer_kind_from_lookup_id(lookup_id)
     if layer_kind == "preset":
         pack_dir = layer.get("pack_dir")
-        if isinstance(pack_dir, Path):
-            manifest_path = pack_dir / "preset.yml"
-            if not manifest_path.is_file():
-                return None
-            try:
-                return manifest_path.relative_to(project_root).as_posix()
-            except ValueError:
-                return None
-        pack_id = layer.get("preset_id") or _extract_lookup_pack_id(lookup_id)
+        pack_id = layer.get("preset_id")
         tier_dir, manifest_name = "presets", "preset.yml"
     elif layer_kind == "extension":
-        ext_dir = layer.get("extension_dir")
-        if isinstance(ext_dir, Path):
-            manifest_path = ext_dir / "extension.yml"
-            if not manifest_path.is_file():
-                return None
-            try:
-                return manifest_path.relative_to(project_root).as_posix()
-            except ValueError:
-                return None
-        pack_id = layer.get("extension_id") or _extract_lookup_pack_id(lookup_id)
+        pack_dir = layer.get("extension_dir")
+        pack_id = layer.get("extension_id")
         tier_dir, manifest_name = "extensions", "extension.yml"
     else:
         return None
-    if not pack_id:
+    if isinstance(pack_dir, Path):
+        manifest_path = pack_dir / manifest_name
+    elif pack_id:
+        manifest_path = project_root / ".specify" / tier_dir / pack_id / manifest_name
+    else:
         return None
-    manifest_path = project_root / ".specify" / tier_dir / pack_id / manifest_name
     if not manifest_path.is_file():
         return None
-    return manifest_path.relative_to(project_root).as_posix()
+    try:
+        return manifest_path.relative_to(project_root).as_posix()
+    except ValueError:
+        return None
 
 
 def _preset_display_name(pack_dir: Path, pack_id: str) -> str:
@@ -475,14 +466,6 @@ class ``PresetManager.list_installed()`` and ``specify preset list`` use —
         return pack_id
 
 
-def _extract_lookup_pack_id(lookup_id: str) -> str | None:
-    """Return the ``sourceId`` segment of a lookupId, or ``None`` if malformed."""
-    parts = lookup_id.split(":")
-    if len(parts) < 4:
-        return None
-    return parts[1]
-
-
 def _build_stack(
     project_root: Path,
     kind: ArtifactKind,
@@ -579,11 +562,12 @@ def _build_stack(
             continue
 
         # Preset layers carry the on-disk directory identity separately from
-        # ``lookupId`` (which may use the manifest-declared ``id:``): prefer
-        # the explicit ``preset_id`` / ``pack_dir`` keys before falling back
-        # to parsing ``lookupId``, so a renamed pack still resolves to the
-        # right on-disk directory for display-name and manifest-path lookup.
-        pack_id = layer.get("preset_id") or _extract_lookup_pack_id(lookup_id) or ""
+        # ``lookupId`` (which may use the manifest-declared ``id:``): use the
+        # explicit ``preset_id`` / ``pack_dir`` keys ``collect_all_layers()``
+        # always sets, never ``lookupId`` parsing, so a renamed pack still
+        # resolves to the right on-disk directory for display-name and
+        # manifest-path lookup.
+        pack_id = layer.get("preset_id") or ""
         pack_dir_layer = layer.get("pack_dir")
         if isinstance(pack_dir_layer, Path):
             pack_dir = pack_dir_layer
@@ -994,9 +978,8 @@ def _iter_project_override_artifacts(
                 continue
             command_layers = resolver.collect_all_layers(name, "command")
             backed_by_command = any(
-                not str(layer.get("lookupId", "")).startswith(
-                    f"{PROJECT_OVERRIDE_LAYER}:"
-                )
+                layer_kind_from_lookup_id(str(layer.get("lookupId", "")))
+                != PROJECT_OVERRIDE_LAYER
                 for layer in command_layers
             )
             is_command = backed_by_command or is_dotted_command_name(name)

tests/test_artifact_command.py

@@ -934,6 +934,8 @@ def test_preset_manifest_path_is_repo_relative(self, tmp_path: Path):
         layer = {
             "lookupId": "preset:my-pack:template:spec-template",
             "path": pack_dir / "spec-template.md",
+            "preset_id": "my-pack",
+            "pack_dir": pack_dir,
         }
         assert (
             _derive_manifest_path(layer, project_root)
@@ -949,17 +951,58 @@ def test_extension_manifest_path_is_repo_relative(self, tmp_path: Path):
         layer = {
             "lookupId": "extension:my-ext:command:speckit.my-ext.go",
             "path": ext_dir / "commands" / "speckit.my-ext.go.md",
+            "extension_id": "my-ext",
+            "extension_dir": ext_dir,
         }
         assert (
             _derive_manifest_path(layer, project_root)
             == ".specify/extensions/my-ext/extension.yml"
         )
 
+    def test_renamed_pack_directory_wins_over_lookup_id_source(self, tmp_path: Path):
+        """The manifest path must track the on-disk directory, never a stale
+        directory guessed from ``lookupId``'s manifest-declared ``sourceId``."""
+        project_root = tmp_path / "proj"
+        pack_dir = project_root / ".specify" / "presets" / "renamed-on-disk"
+        pack_dir.mkdir(parents=True)
+        (pack_dir / "preset.yml").write_text("id: original-manifest-id\n", encoding="utf-8")
+        # A stale directory matching the manifest id must not exist, so a
+        # lookupId-based guess would resolve to a nonexistent manifest.
+        stale_dir = project_root / ".specify" / "presets" / "original-manifest-id"
+        assert not stale_dir.exists()
+
+        layer = {
+            "lookupId": "preset:original-manifest-id:template:spec-template",
+            "path": pack_dir / "spec-template.md",
+            "preset_id": "renamed-on-disk",
+            "pack_dir": pack_dir,
+        }
+        assert (
+            _derive_manifest_path(layer, project_root)
+            == ".specify/presets/renamed-on-disk/preset.yml"
+        )
+
     def test_missing_manifest_file_is_none(self, tmp_path: Path):
         project_root = tmp_path / "proj"
         pack_dir = project_root / ".specify" / "presets" / "my-pack"
         pack_dir.mkdir(parents=True)
 
+        layer = {
+            "lookupId": "preset:my-pack:template:spec-template",
+            "path": pack_dir / "spec-template.md",
+            "preset_id": "my-pack",
+            "pack_dir": pack_dir,
+        }
+        assert _derive_manifest_path(layer, project_root) is None
+
+    def test_missing_provenance_keys_is_none(self, tmp_path: Path):
+        """Without explicit ``preset_id``/``pack_dir``, no path is guessed from
+        ``lookupId`` — the caller gets ``None`` instead of a wrong path."""
+        project_root = tmp_path / "proj"
+        pack_dir = project_root / ".specify" / "presets" / "my-pack"
+        pack_dir.mkdir(parents=True)
+        (pack_dir / "preset.yml").write_text("id: my-pack\n", encoding="utf-8")
+
         layer = {
             "lookupId": "preset:my-pack:template:spec-template",
             "path": pack_dir / "spec-template.md",

tests/test_contribution_ids.py

@@ -159,6 +159,10 @@ class TestLayerKindFromLookupId:
             ("preset:speckit-core:template:spec-template", "preset"),
             ("extension:speckit-git:script:post-commit", "extension"),
             (f"{PROJECT_OVERRIDE_LAYER}:_:template:spec-template", PROJECT_OVERRIDE_LAYER),
+            (
+                "extension:speckit-git:hook:before_specify:speckit.git.branch",
+                "extension",
+            ),
         ],
     )
     def test_recognized_layer_prefixes(self, lookup_id, expected):
@@ -171,6 +175,11 @@ def test_recognized_layer_prefixes(self, lookup_id, expected):
             "bogus:_:command:speckit.plan",
             "core",
             ":_:command:speckit.plan",
+            "core:not-an-id",
+            "preset:x",
+            "core:_:command",
+            "extension:speckit-git:hook:before_specify",
+            "core::command:speckit.plan",
         ],
     )
     def test_unrecognized_or_malformed_returns_none(self, lookup_id):